Auto merge of #14793 - epage:test-compare, r=weihanglo

docs(test):  Document Execs assertions based on port effort

### What does this PR try to resolve?

Closes #14039

A lot of this was pulled from #14039, common assertions people need to write, and from seeing people not noticing features that exist.

I'm leaving behind the `contains` assertions, rather than finding a way to make them work with snapbox.  Thankfully, snapbox is designed to let people build their own thing (see #14790).  I considered reusing snapbox's `[..]` matches but the code here is pretty minimal, the logic is similar enough, and I don't have a great abstraction for snapbox for it.  If there was more of a need, I'd make something work in snapbox.  As such, the `contains` assertions are no longer deprecated.

While doing this last pass through, I did some polish on the code as well.

### How should we test and review this PR?

### Additional information

Author: bors

crates/cargo-test-support/src/compare.rs

@@ -331,27 +331,6 @@ static E2E_LITERAL_REDACTIONS: &[(&str, &str)] = &[
     ("[OPENING]", "     Opening"),
 ];
 
-/// Normalizes the output so that it can be compared against the expected value.
-fn normalize_actual(content: &str, redactions: &snapbox::Redactions) -> String {
-    use snapbox::filter::Filter as _;
-    let content = snapbox::filter::FilterPaths.filter(content.into_data());
-    let content = snapbox::filter::FilterNewlines.filter(content);
-    let content = content.render().expect("came in as a String");
-    let content = redactions.redact(&content);
-    content
-}
-
-/// Normalizes the expected string so that it can be compared against the actual output.
-fn normalize_expected(content: &str, redactions: &snapbox::Redactions) -> String {
-    use snapbox::filter::Filter as _;
-    let content = snapbox::filter::FilterPaths.filter(content.into_data());
-    let content = snapbox::filter::FilterNewlines.filter(content);
-    // Remove any conditionally absent redactions like `[EXE]`
-    let content = content.render().expect("came in as a String");
-    let content = redactions.clear_unused(&content);
-    content.into_owned()
-}
-
 /// Checks that the given string contains the given contiguous lines
 /// somewhere.
 ///
@@ -364,12 +343,12 @@ pub(crate) fn match_contains(
     let expected = normalize_expected(expected, redactions);
     let actual = normalize_actual(actual, redactions);
     let e: Vec<_> = expected.lines().map(|line| WildStr::new(line)).collect();
-    let a: Vec<_> = actual.lines().map(|line| WildStr::new(line)).collect();
+    let a: Vec<_> = actual.lines().collect();
     if e.len() == 0 {
         bail!("expected length must not be zero");
     }
     for window in a.windows(e.len()) {
-        if window == e {
+        if e == window {
             return Ok(());
         }
     }
@@ -428,7 +407,6 @@ pub(crate) fn match_with_without(
 
     let matches: Vec<_> = actual
         .lines()
-        .map(WildStr::new)
         .filter(|line| with_wild.iter().all(|with| with == line))
         .filter(|line| !without_wild.iter().any(|without| without == line))
         .collect();
@@ -457,28 +435,48 @@ pub(crate) fn match_with_without(
     }
 }
 
+/// Normalizes the output so that it can be compared against the expected value.
+fn normalize_actual(content: &str, redactions: &snapbox::Redactions) -> String {
+    use snapbox::filter::Filter as _;
+    let content = snapbox::filter::FilterPaths.filter(content.into_data());
+    let content = snapbox::filter::FilterNewlines.filter(content);
+    let content = content.render().expect("came in as a String");
+    let content = redactions.redact(&content);
+    content
+}
+
+/// Normalizes the expected string so that it can be compared against the actual output.
+fn normalize_expected(content: &str, redactions: &snapbox::Redactions) -> String {
+    use snapbox::filter::Filter as _;
+    let content = snapbox::filter::FilterPaths.filter(content.into_data());
+    let content = snapbox::filter::FilterNewlines.filter(content);
+    // Remove any conditionally absent redactions like `[EXE]`
+    let content = content.render().expect("came in as a String");
+    let content = redactions.clear_unused(&content);
+    content.into_owned()
+}
+
 /// A single line string that supports `[..]` wildcard matching.
-pub(crate) struct WildStr<'a> {
+struct WildStr<'a> {
     has_meta: bool,
     line: &'a str,
 }
 
 impl<'a> WildStr<'a> {
-    pub fn new(line: &'a str) -> WildStr<'a> {
+    fn new(line: &'a str) -> WildStr<'a> {
         WildStr {
             has_meta: line.contains("[..]"),
             line,
         }
     }
 }
 
-impl<'a> PartialEq for WildStr<'a> {
-    fn eq(&self, other: &Self) -> bool {
-        match (self.has_meta, other.has_meta) {
-            (false, false) => self.line == other.line,
-            (true, false) => meta_cmp(self.line, other.line),
-            (false, true) => meta_cmp(other.line, self.line),
-            (true, true) => panic!("both lines cannot have [..]"),
+impl PartialEq<&str> for WildStr<'_> {
+    fn eq(&self, other: &&str) -> bool {
+        if self.has_meta {
+            meta_cmp(self.line, other)
+        } else {
+            self.line == *other
         }
     }
 }
@@ -666,10 +664,10 @@ mod test {
             ("[..]", "a b"),
             ("[..]b", "a b"),
         ] {
-            assert_eq!(WildStr::new(a), WildStr::new(b));
+            assert_eq!(WildStr::new(a), b);
         }
         for (a, b) in &[("[..]b", "c"), ("b", "c"), ("b", "cb")] {
-            assert_ne!(WildStr::new(a), WildStr::new(b));
+            assert_ne!(WildStr::new(a), b);
         }
     }
 

crates/cargo-test-support/src/lib.rs

@@ -667,6 +667,57 @@ impl Execs {
     /// Verifies that stdout is equal to the given lines.
     ///
     /// See [`compare::assert_e2e`] for assertion details.
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer passing in [`str!`] for `expected` to get snapshot updating.
+    ///
+    /// If `format!` is needed for content that changes from run to run that you don't care about,
+    /// consider whether you could have [`compare::assert_e2e`] redact the content.
+    /// If nothing else, a wildcard (`[..]`, `...`) may be useful.
+    ///
+    /// However, `""` may be preferred for intentionally empty output so people don't accidentally
+    /// bless a change.
+    ///
+    /// </div>
+    ///
+    /// # Examples
+    ///
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stdout_data(str![r#"
+    /// Hello world!
+    /// "#]);
+    /// ```
+    ///
+    /// Non-deterministic compiler output
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stdout_data(str![r#"
+    /// [COMPILING] foo
+    /// [COMPILING] bar
+    /// "#].unordered());
+    /// ```
+    ///
+    /// jsonlines
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stdout_data(str![r#"
+    /// [
+    ///   {},
+    ///   {}
+    /// ]
+    /// "#].is_json().against_jsonlines());
+    /// ```
     pub fn with_stdout_data(&mut self, expected: impl snapbox::IntoData) -> &mut Self {
         self.expect_stdout_data = Some(expected.into_data());
         self
@@ -675,6 +726,57 @@ impl Execs {
     /// Verifies that stderr is equal to the given lines.
     ///
     /// See [`compare::assert_e2e`] for assertion details.
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer passing in [`str!`] for `expected` to get snapshot updating.
+    ///
+    /// If `format!` is needed for content that changes from run to run that you don't care about,
+    /// consider whether you could have [`compare::assert_e2e`] redact the content.
+    /// If nothing else, a wildcard (`[..]`, `...`) may be useful.
+    ///
+    /// However, `""` may be preferred for intentionally empty output so people don't accidentally
+    /// bless a change.
+    ///
+    /// </div>
+    ///
+    /// # Examples
+    ///
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stderr_data(str![r#"
+    /// Hello world!
+    /// "#]);
+    /// ```
+    ///
+    /// Non-deterministic compiler output
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stderr_data(str![r#"
+    /// [COMPILING] foo
+    /// [COMPILING] bar
+    /// "#].unordered());
+    /// ```
+    ///
+    /// jsonlines
+    /// ```no_run
+    /// use cargo_test_support::prelude::*;
+    /// use cargo_test_support::str;
+    /// use cargo_test_support::execs;
+    ///
+    /// execs().with_stderr_data(str![r#"
+    /// [
+    ///   {},
+    ///   {}
+    /// ]
+    /// "#].is_json().against_jsonlines());
+    /// ```
     pub fn with_stderr_data(&mut self, expected: impl snapbox::IntoData) -> &mut Self {
         self.expect_stderr_data = Some(expected.into_data());
         self
@@ -706,7 +808,14 @@ impl Execs {
     /// its output.
     ///
     /// See [`compare`] for supported patterns.
-    #[deprecated(note = "replaced with `Execs::with_stdout_data(expected)`")]
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer [`Execs::with_stdout_data`] where possible.
+    /// - `expected` cannot be snapshotted
+    /// - `expected` can end up being ambiguous, causing the assertion to succeed when it should fail
+    ///
+    /// </div>
     pub fn with_stdout_contains<S: ToString>(&mut self, expected: S) -> &mut Self {
         self.expect_stdout_contains.push(expected.to_string());
         self
@@ -716,7 +825,14 @@ impl Execs {
     /// its output.
     ///
     /// See [`compare`] for supported patterns.
-    #[deprecated(note = "replaced with `Execs::with_stderr_data(expected)`")]
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer [`Execs::with_stderr_data`] where possible.
+    /// - `expected` cannot be snapshotted
+    /// - `expected` can end up being ambiguous, causing the assertion to succeed when it should fail
+    ///
+    /// </div>
     pub fn with_stderr_contains<S: ToString>(&mut self, expected: S) -> &mut Self {
         self.expect_stderr_contains.push(expected.to_string());
         self
@@ -727,7 +843,18 @@ impl Execs {
     /// See [`compare`] for supported patterns.
     ///
     /// See note on [`Self::with_stderr_does_not_contain`].
-    #[deprecated]
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer [`Execs::with_stdout_data`] where possible.
+    /// - `expected` cannot be snapshotted
+    /// - The absence of `expected` can either mean success or that the string being looked for
+    ///   changed.
+    ///
+    /// To mitigate this, consider matching this up with
+    /// [`Execs::with_stdout_contains`].
+    ///
+    /// </div>
     pub fn with_stdout_does_not_contain<S: ToString>(&mut self, expected: S) -> &mut Self {
         self.expect_stdout_not_contains.push(expected.to_string());
         self
@@ -737,12 +864,18 @@ impl Execs {
     ///
     /// See [`compare`] for supported patterns.
     ///
-    /// Care should be taken when using this method because there is a
-    /// limitless number of possible things that *won't* appear. A typo means
-    /// your test will pass without verifying the correct behavior. If
-    /// possible, write the test first so that it fails, and then implement
-    /// your fix/feature to make it pass.
-    #[deprecated]
+    /// <div class="warning">
+    ///
+    /// Prefer [`Execs::with_stdout_data`] where possible.
+    /// - `expected` cannot be snapshotted
+    /// - The absence of `expected` can either mean success or that the string being looked for
+    ///   changed.
+    ///
+    /// To mitigate this, consider either matching this up with
+    /// [`Execs::with_stdout_contains`] or replace it
+    /// with [`Execs::with_stderr_line_without`].
+    ///
+    /// </div>
     pub fn with_stderr_does_not_contain<S: ToString>(&mut self, expected: S) -> &mut Self {
         self.expect_stderr_not_contains.push(expected.to_string());
         self
@@ -751,7 +884,18 @@ impl Execs {
     /// Verify that a particular line appears in stderr with and without the
     /// given substrings. Exactly one line must match.
     ///
-    /// The substrings are matched as `contains`. Example:
+    /// The substrings are matched as `contains`.
+    ///
+    /// <div class="warning">
+    ///
+    /// Prefer [`Execs::with_stdout_data`] where possible.
+    /// - `with` cannot be snapshotted
+    /// - The absence of `without`` can either mean success or that the string being looked for
+    ///   changed.
+    ///
+    /// </div>
+    ///
+    /// # Example
     ///
     /// ```no_run
     /// use cargo_test_support::execs;
@@ -768,9 +912,6 @@ impl Execs {
     /// This will check that a build line includes `-C opt-level=3` but does
     /// not contain `-C debuginfo` or `-C incremental`.
     ///
-    /// Be careful writing the `without` fragments, see note in
-    /// `with_stderr_does_not_contain`.
-    #[deprecated]
     pub fn with_stderr_line_without<S: ToString>(
         &mut self,
         with: &[S],

tests/testsuite/artifact_dep.rs

@@ -687,7 +687,6 @@ fn build_script_with_bin_artifact_and_lib_false() {
         )
         .build();
 
-    #[expect(deprecated)]
     p.cargo("build -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
         .with_status(101)
@@ -731,7 +730,6 @@ fn lib_with_bin_artifact_and_lib_false() {
         )
         .build();
 
-    #[expect(deprecated)]
     p.cargo("build -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
         .with_status(101)
@@ -1117,7 +1115,6 @@ fn build_script_deps_adopt_specified_target_unconditionally() {
         .file("bar/src/lib.rs", "pub fn doit() {}")
         .build();
 
-    #[expect(deprecated)]
     p.cargo("check -v -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
         .with_stderr_does_not_contain(
@@ -1233,7 +1230,6 @@ fn non_build_script_deps_adopt_specified_target_unconditionally() {
         .file("bar/src/lib.rs", "pub fn doit() {}")
         .build();
 
-    #[expect(deprecated)]
     p.cargo("check -v -Z bindeps")
         .masquerade_as_nightly_cargo(&["bindeps"])
         .with_stderr_contains(
@@ -1378,7 +1374,6 @@ fn build_script_deps_adopts_target_platform_if_target_equals_target() {
         .build();
 
     let alternate_target = cross_compile::alternate();
-    #[expect(deprecated)]
     p.cargo("check -v -Z bindeps --target")
         .arg(alternate_target)
         .masquerade_as_nightly_cargo(&["bindeps"])