Auto merge of #11028 - ehuss:test-errors, r=weihanglo

Rework test error handling

This reworks how errors are handled when running tests and benchmarks. There were some cases where Cargo was eating the actual error and not displaying it. For example, if a test process fails to launch, it only displayed the `could not execute process` message, but didn't explain why it failed to execute. This fixes it to ensure that the full error chain is displayed.

This also tries to simplify how the errors are handled, and makes them more uniform across `test` and `bench`, and with doctests.

This also changes the `--no-fail-fast` behavior to report errors as they happen instead of grouped at the end (and prints a summary at the end). This helps to make it clearer when a nonstandard error happens. For example, before:

```
     Running tests/t1.rs (target/debug/deps/t1-bb449dfa37379ba1)

running 1 test
     Running tests/t2.rs (target/debug/deps/t2-1770ae8367bc97ce)

running 1 test
test bar ... FAILED

failures:

---- bar stdout ----
thread 'bar' panicked at 'y', tests/t2.rs:3:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    bar

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed.

Caused by:
  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t2-1770ae8367bc97ce` (exit status: 101)
```

and the changes to that are:

```diff
`@@` -1,6 +1,10 `@@`
      Running tests/t1.rs (target/debug/deps/t1-bb449dfa37379ba1)

 running 1 test
+error: test failed, to rerun pass `--test t1`
+
+Caused by:
+  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
      Running tests/t2.rs (target/debug/deps/t2-1770ae8367bc97ce)

 running 1 test
`@@` -18,8 +22,7 `@@`

 test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

-error: test failed.
-
-Caused by:
-  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t1-bb449dfa37379ba1` (signal: 11, SIGSEGV: invalid memory reference)
-  process didn't exit successfully: `/Users/eric/Temp/z12/target/debug/deps/t2-1770ae8367bc97ce` (exit status: 101)
+error: test failed, to rerun pass `--test t2`
+error: 2 targets failed:
+    `--test t1`
+    `--test t2`
```

In the first example, when it says `Running tests/t1.rs`, there is no error message displayed until after all the tests finish, and that error message is not associated with the original test. This also includes the "to rerun" hint with `--no-fail-fast`.

src/bin/cargo/commands/bench.rs

@@ -73,12 +73,5 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let bench_args = bench_args.chain(args.get_many::<String>("args").unwrap_or_default());
     let bench_args = bench_args.map(String::as_str).collect::<Vec<_>>();
 
-    let err = ops::run_benches(&ws, &ops, &bench_args)?;
-    match err {
-        None => Ok(()),
-        Some(err) => Err(match err.code {
-            Some(i) => CliError::new(anyhow::format_err!("bench failed"), i),
-            None => CliError::new(err.into(), 101),
-        }),
-    }
+    ops::run_benches(&ws, &ops, &bench_args)
 }

src/bin/cargo/commands/test.rs

@@ -1,5 +1,4 @@
 use crate::command_prelude::*;
-use anyhow::Error;
 use cargo::ops;
 
 pub fn cli() -> App {
@@ -110,18 +109,5 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
         compile_opts,
     };
 
-    let err = ops::run_tests(&ws, &ops, &test_args)?;
-    match err {
-        None => Ok(()),
-        Some(err) => {
-            let context = anyhow::format_err!("{}", err.hint(&ws, &ops.compile_opts));
-            let e = match err.code {
-                // Don't show "process didn't exit successfully" for simple errors.
-                Some(i) if cargo_util::is_simple_exit_code(i) => CliError::new(context, i),
-                Some(i) => CliError::new(Error::from(err).context(context), i),
-                None => CliError::new(Error::from(err).context(context), 101),
-            };
-            Err(e)
-        }
-    }
+    ops::run_tests(&ws, &ops, &test_args)
 }

src/cargo/ops/cargo_test.rs

@@ -3,9 +3,11 @@ use crate::core::shell::Verbosity;
 use crate::core::{TargetKind, Workspace};
 use crate::ops;
 use crate::util::errors::CargoResult;
-use crate::util::{add_path_args, CargoTestError, Config, Test};
+use crate::util::{add_path_args, CliError, CliResult, Config};
+use anyhow::format_err;
 use cargo_util::{ProcessBuilder, ProcessError};
 use std::ffi::OsString;
+use std::fmt::Write;
 use std::path::{Path, PathBuf};
 
 pub struct TestOptions {
@@ -14,61 +16,87 @@ pub struct TestOptions {
     pub no_fail_fast: bool,
 }
 
-pub fn run_tests(
-    ws: &Workspace<'_>,
-    options: &TestOptions,
-    test_args: &[&str],
-) -> CargoResult<Option<CargoTestError>> {
+/// The kind of test.
+///
+/// This is needed because `Unit` does not track whether or not something is a
+/// benchmark.
+#[derive(Copy, Clone)]
+enum TestKind {
+    Test,
+    Bench,
+    Doctest,
+}
+
+/// A unit that failed to run.
+struct UnitTestError {
+    unit: Unit,
+    kind: TestKind,
+}
+
+impl UnitTestError {
+    /// Returns the CLI args needed to target this unit.
+    fn cli_args(&self, ws: &Workspace<'_>, opts: &ops::CompileOptions) -> String {
+        let mut args = if opts.spec.needs_spec_flag(ws) {
+            format!("-p {} ", self.unit.pkg.name())
+        } else {
+            String::new()
+        };
+        let mut add = |which| write!(args, "--{which} {}", self.unit.target.name()).unwrap();
+
+        match self.kind {
+            TestKind::Test | TestKind::Bench => match self.unit.target.kind() {
+                TargetKind::Lib(_) => args.push_str("--lib"),
+                TargetKind::Bin => add("bin"),
+                TargetKind::Test => add("test"),
+                TargetKind::Bench => add("bench"),
+                TargetKind::ExampleLib(_) | TargetKind::ExampleBin => add("example"),
+                TargetKind::CustomBuild => panic!("unexpected CustomBuild kind"),
+            },
+            TestKind::Doctest => args.push_str("--doc"),
+        }
+        args
+    }
+}
+
+/// Compiles and runs tests.
+///
+/// On error, the returned [`CliError`] will have the appropriate process exit
+/// code that Cargo should use.
+pub fn run_tests(ws: &Workspace<'_>, options: &TestOptions, test_args: &[&str]) -> CliResult {
     let compilation = compile_tests(ws, options)?;
 
     if options.no_run {
         if !options.compile_opts.build_config.emit_json() {
             display_no_run_information(ws, test_args, &compilation, "unittests")?;
         }
-
-        return Ok(None);
+        return Ok(());
     }
-    let (test, mut errors) = run_unit_tests(ws.config(), options, test_args, &compilation)?;
+    let mut errors = run_unit_tests(ws, options, test_args, &compilation, TestKind::Test)?;
 
-    // If we have an error and want to fail fast, then return.
-    if !errors.is_empty() && !options.no_fail_fast {
-        return Ok(Some(CargoTestError::new(test, errors)));
-    }
-
-    let (doctest, docerrors) = run_doc_tests(ws, options, test_args, &compilation)?;
-    let test = if docerrors.is_empty() { test } else { doctest };
-    errors.extend(docerrors);
-    if errors.is_empty() {
-        Ok(None)
-    } else {
-        Ok(Some(CargoTestError::new(test, errors)))
-    }
+    let doctest_errors = run_doc_tests(ws, options, test_args, &compilation)?;
+    errors.extend(doctest_errors);
+    no_fail_fast_err(ws, &options.compile_opts, &errors)
 }
 
-pub fn run_benches(
-    ws: &Workspace<'_>,
-    options: &TestOptions,
-    args: &[&str],
-) -> CargoResult<Option<CargoTestError>> {
+/// Compiles and runs benchmarks.
+///
+/// On error, the returned [`CliError`] will have the appropriate process exit
+/// code that Cargo should use.
+pub fn run_benches(ws: &Workspace<'_>, options: &TestOptions, args: &[&str]) -> CliResult {
     let compilation = compile_tests(ws, options)?;
 
     if options.no_run {
         if !options.compile_opts.build_config.emit_json() {
             display_no_run_information(ws, args, &compilation, "benches")?;
         }
-
-        return Ok(None);
+        return Ok(());
     }
 
     let mut args = args.to_vec();
     args.push("--bench");
 
-    let (test, errors) = run_unit_tests(ws.config(), options, &args, &compilation)?;
-
-    match errors.len() {
-        0 => Ok(None),
-        _ => Ok(Some(CargoTestError::new(test, errors))),
-    }
+    let errors = run_unit_tests(ws, options, &args, &compilation, TestKind::Bench)?;
+    no_fail_fast_err(ws, &options.compile_opts, &errors)
 }
 
 fn compile_tests<'a>(ws: &Workspace<'a>, options: &TestOptions) -> CargoResult<Compilation<'a>> {
@@ -78,12 +106,17 @@ fn compile_tests<'a>(ws: &Workspace<'a>, options: &TestOptions) -> CargoResult<C
 }
 
 /// Runs the unit and integration tests of a package.
+///
+/// Returns a `Vec` of tests that failed when `--no-fail-fast` is used.
+/// If `--no-fail-fast` is *not* used, then this returns an `Err`.
 fn run_unit_tests(
-    config: &Config,
+    ws: &Workspace<'_>,
     options: &TestOptions,
     test_args: &[&str],
     compilation: &Compilation<'_>,
-) -> CargoResult<(Test, Vec<ProcessError>)> {
+    test_kind: TestKind,
+) -> Result<Vec<UnitTestError>, CliError> {
+    let config = ws.config();
     let cwd = config.cwd();
     let mut errors = Vec::new();
 
@@ -110,46 +143,32 @@ fn run_unit_tests(
             .shell()
             .verbose(|shell| shell.status("Running", &cmd))?;
 
-        let result = cmd.exec();
-
-        if let Err(e) = result {
-            let e = e.downcast::<ProcessError>()?;
-            errors.push((
-                unit.target.kind().clone(),
-                unit.target.name().to_string(),
-                unit.pkg.name().to_string(),
-                e,
-            ));
+        if let Err(e) = cmd.exec() {
+            let code = fail_fast_code(&e);
+            let unit_err = UnitTestError {
+                unit: unit.clone(),
+                kind: test_kind,
+            };
+            report_test_error(ws, &options.compile_opts, &unit_err, e);
+            errors.push(unit_err);
             if !options.no_fail_fast {
-                break;
+                return Err(CliError::code(code));
             }
         }
     }
-
-    if errors.len() == 1 {
-        let (kind, name, pkg_name, e) = errors.pop().unwrap();
-        Ok((
-            Test::UnitTest {
-                kind,
-                name,
-                pkg_name,
-            },
-            vec![e],
-        ))
-    } else {
-        Ok((
-            Test::Multiple,
-            errors.into_iter().map(|(_, _, _, e)| e).collect(),
-        ))
-    }
+    Ok(errors)
 }
 
+/// Runs doc tests.
+///
+/// Returns a `Vec` of tests that failed when `--no-fail-fast` is used.
+/// If `--no-fail-fast` is *not* used, then this returns an `Err`.
 fn run_doc_tests(
     ws: &Workspace<'_>,
     options: &TestOptions,
     test_args: &[&str],
     compilation: &Compilation<'_>,
-) -> CargoResult<(Test, Vec<ProcessError>)> {
+) -> Result<Vec<UnitTestError>, CliError> {
     let config = ws.config();
     let mut errors = Vec::new();
     let doctest_xcompile = config.cli_unstable().doctest_xcompile;
@@ -258,16 +277,24 @@ fn run_doc_tests(
             .shell()
             .verbose(|shell| shell.status("Running", p.to_string()))?;
         if let Err(e) = p.exec() {
-            let e = e.downcast::<ProcessError>()?;
-            errors.push(e);
+            let code = fail_fast_code(&e);
+            let unit_err = UnitTestError {
+                unit: unit.clone(),
+                kind: TestKind::Doctest,
+            };
+            report_test_error(ws, &options.compile_opts, &unit_err, e);
+            errors.push(unit_err);
             if !options.no_fail_fast {
-                return Ok((Test::Doc, errors));
+                return Err(CliError::code(code));
             }
         }
     }
-    Ok((Test::Doc, errors))
+    Ok(errors)
 }
 
+/// Displays human-readable descriptions of the test executables.
+///
+/// This is used when `cargo test --no-run` is used.
 fn display_no_run_information(
     ws: &Workspace<'_>,
     test_args: &[&str],
@@ -303,6 +330,11 @@ fn display_no_run_information(
     return Ok(());
 }
 
+/// Creates a [`ProcessBuilder`] for executing a single test.
+///
+/// Returns a tuple `(exe_display, process)` where `exe_display` is a string
+/// to display that describes the executable path in a human-readable form.
+/// `process` is the `ProcessBuilder` to use for executing the test.
 fn cmd_builds(
     config: &Config,
     cwd: &Path,
@@ -341,3 +373,67 @@ fn cmd_builds(
 
     Ok((exe_display, cmd))
 }
+
+/// Returns the error code to use when *not* using `--no-fail-fast`.
+///
+/// Cargo will return the error code from the test process itself. If some
+/// other error happened (like a failure to launch the process), then it will
+/// return a standard 101 error code.
+///
+/// When using `--no-fail-fast`, Cargo always uses the 101 exit code (since
+/// there may not be just one process to report).
+fn fail_fast_code(error: &anyhow::Error) -> i32 {
+    if let Some(proc_err) = error.downcast_ref::<ProcessError>() {
+        if let Some(code) = proc_err.code {
+            return code;
+        }
+    }
+    101
+}
+
+/// Returns the `CliError` when using `--no-fail-fast` and there is at least
+/// one error.
+fn no_fail_fast_err(
+    ws: &Workspace<'_>,
+    opts: &ops::CompileOptions,
+    errors: &[UnitTestError],
+) -> CliResult {
+    // TODO: This could be improved by combining the flags on a single line when feasible.
+    let args: Vec<_> = errors
+        .iter()
+        .map(|unit_err| format!("    `{}`", unit_err.cli_args(ws, opts)))
+        .collect();
+    let message = match errors.len() {
+        0 => return Ok(()),
+        1 => format!("1 target failed:\n{}", args.join("\n")),
+        n => format!("{n} targets failed:\n{}", args.join("\n")),
+    };
+    Err(anyhow::Error::msg(message).into())
+}
+
+/// Displays an error on the console about a test failure.
+fn report_test_error(
+    ws: &Workspace<'_>,
+    opts: &ops::CompileOptions,
+    unit_err: &UnitTestError,
+    test_error: anyhow::Error,
+) {
+    let which = match unit_err.kind {
+        TestKind::Test => "test failed",
+        TestKind::Bench => "bench failed",
+        TestKind::Doctest => "doctest failed",
+    };
+
+    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
+        .downcast_ref::<ProcessError>()
+        .and_then(|proc_err| proc_err.code)
+        .map_or(false, |code| code == 101);
+    if !is_simple {
+        err = test_error.context(err);
+    }
+
+    crate::display_error(&err, &mut ws.config().shell());
+}