Auto merge of #12463 - stupendoussuperpowers:nocapture_test_msg, r=epage

prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal.

Fixes #10855

As per the discussion on this issue, we want to prompt the user to use `--nocapture` if a test is terminated abnormally. The motivation for this change is described in the issue.

We check for 3 things before we display this flag. -
- `!is_simple` (if the test ended with a non 101 status code)
- `harness` (if the standard test harness was used), and
- `!nocapture` (whether or not the `--nocapture` flag was already passed to the test)

There's further tests added to `test::nonzero_exit_status` that check that the `stderr` is correct for the various combinations possible when a test ends with a non-101 status code.

The new expected behavior is -
- Display `--nocapture` note for only non-zero exit statuses, when the `--nocapture` flag is not passed.
- Only display the note if we use a standard test harness since custom test harnesses do not implement the `--nocapture` flag.

To implement the check for the `--nocapture` flag, the function definition for `report_test_errors` was changed to add the `test_args: &[&str]` parameter. This parameter is passed from the immediate calling function. This private function is only called twice change and is not causing regression after making the appropriate changes to both the places it's called in.

Author: bors

src/cargo/ops/cargo_test.rs

@@ -149,7 +149,7 @@ fn run_unit_tests(
                 unit: unit.clone(),
                 kind: test_kind,
             };
-            report_test_error(ws, &options.compile_opts, &unit_err, e);
+            report_test_error(ws, test_args, &options.compile_opts, &unit_err, e);
             errors.push(unit_err);
             if !options.no_fail_fast {
                 return Err(CliError::code(code));
@@ -275,7 +275,7 @@ fn run_doc_tests(
                 unit: unit.clone(),
                 kind: TestKind::Doctest,
             };
-            report_test_error(ws, &options.compile_opts, &unit_err, e);
+            report_test_error(ws, test_args, &options.compile_opts, &unit_err, e);
             errors.push(unit_err);
             if !options.no_fail_fast {
                 return Err(CliError::code(code));
@@ -407,6 +407,7 @@ fn no_fail_fast_err(
 /// Displays an error on the console about a test failure.
 fn report_test_error(
     ws: &Workspace<'_>,
+    test_args: &[&str],
     opts: &ops::CompileOptions,
     unit_err: &UnitTestError,
     test_error: anyhow::Error,
@@ -420,13 +421,23 @@ fn report_test_error(
     let mut err = format_err!("{}, to rerun pass `{}`", which, unit_err.cli_args(ws, opts));
     // Don't show "process didn't exit successfully" for simple errors.
     // libtest exits with 101 for normal errors.
-    let is_simple = test_error
+    let (is_simple, executed) = test_error
         .downcast_ref::<ProcessError>()
         .and_then(|proc_err| proc_err.code)
-        .map_or(false, |code| code == 101);
+        .map_or((false, false), |code| (code == 101, true));
+
     if !is_simple {
         err = test_error.context(err);
     }
 
     crate::display_error(&err, &mut ws.config().shell());
+
+    let harness: bool = unit_err.unit.target.harness();
+    let nocapture: bool = test_args.contains(&"--nocapture");
+
+    if !is_simple && executed && harness && !nocapture {
+        drop(ws.config().shell().note(
+            "test exited abnormally; to see the full output pass --nocapture to the harness.",
+        ));
+    }
 }

tests/testsuite/test.rs

@@ -4792,6 +4792,21 @@ error: test failed, to rerun pass `--test t1`
 [RUNNING] tests/t2.rs (target/debug/deps/t2[..])
 error: test failed, to rerun pass `--test t2`
 
+Caused by:
+  process didn't exit successfully: `[ROOT]/foo/target/debug/deps/t2[..]` (exit [..]: 4)
+note: test exited abnormally; to see the full output pass --nocapture to the harness.
+",
+        )
+        .with_status(4)
+        .run();
+
+    p.cargo("test --test t2 -- --nocapture")
+        .with_stderr(
+            "\
+[FINISHED] test [..]
+[RUNNING] tests/t2.rs (target/debug/deps/t2[..])
+error: test failed, to rerun pass `--test t2`
+
 Caused by:
   process didn't exit successfully: `[ROOT]/foo/target/debug/deps/t2[..]` (exit [..]: 4)
 ",
@@ -4811,11 +4826,20 @@ error: test failed, to rerun pass `--test t2`
 
 Caused by:
   process didn't exit successfully: `[ROOT]/foo/target/debug/deps/t2[..]` (exit [..]: 4)
+note: test exited abnormally; to see the full output pass --nocapture to the harness.
 error: 2 targets failed:
     `--test t1`
     `--test t2`
 ",
         )
         .with_status(101)
         .run();
+
+    p.cargo("test --no-fail-fast -- --nocapture")
+    .with_stderr_does_not_contain("test exited abnormally; to see the full output pass --nocapture to the harness.")
+    .with_stderr_contains("[..]thread 't' panicked [..] tests/t1[..]")
+    .with_stderr_contains("note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace")
+    .with_stderr_contains("[..]process didn't exit successfully: `[ROOT]/foo/target/debug/deps/t2[..]` (exit [..]: 4)")
+    .with_status(101)
+    .run();
 }