Skip to content

Commit 23735d4

Browse files
committed
Rework test error handling
1 parent e5ec3a8 commit 23735d4

File tree

7 files changed

+427
-209
lines changed

7 files changed

+427
-209
lines changed

src/bin/cargo/commands/bench.rs

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,5 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
7373
let bench_args = bench_args.chain(args.get_many::<String>("args").unwrap_or_default());
7474
let bench_args = bench_args.map(String::as_str).collect::<Vec<_>>();
7575

76-
let err = ops::run_benches(&ws, &ops, &bench_args)?;
77-
match err {
78-
None => Ok(()),
79-
Some(err) => Err(match err.code {
80-
Some(i) => CliError::new(anyhow::format_err!("bench failed"), i),
81-
None => CliError::new(err.into(), 101),
82-
}),
83-
}
76+
ops::run_benches(&ws, &ops, &bench_args)
8477
}

src/bin/cargo/commands/test.rs

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use crate::command_prelude::*;
2-
use anyhow::Error;
32
use cargo::ops;
43

54
pub fn cli() -> App {
@@ -110,18 +109,5 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
110109
compile_opts,
111110
};
112111

113-
let err = ops::run_tests(&ws, &ops, &test_args)?;
114-
match err {
115-
None => Ok(()),
116-
Some(err) => {
117-
let context = anyhow::format_err!("{}", err.hint(&ws, &ops.compile_opts));
118-
let e = match err.code {
119-
// Don't show "process didn't exit successfully" for simple errors.
120-
Some(i) if cargo_util::is_simple_exit_code(i) => CliError::new(context, i),
121-
Some(i) => CliError::new(Error::from(err).context(context), i),
122-
None => CliError::new(Error::from(err).context(context), 101),
123-
};
124-
Err(e)
125-
}
126-
}
112+
ops::run_tests(&ws, &ops, &test_args)
127113
}

src/cargo/ops/cargo_test.rs

Lines changed: 166 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@ use crate::core::shell::Verbosity;
33
use crate::core::{TargetKind, Workspace};
44
use crate::ops;
55
use crate::util::errors::CargoResult;
6-
use crate::util::{add_path_args, CargoTestError, Config, Test};
6+
use crate::util::{add_path_args, CliError, CliResult, Config};
7+
use anyhow::format_err;
78
use cargo_util::{ProcessBuilder, ProcessError};
89
use std::ffi::OsString;
10+
use std::fmt::Write;
911
use std::path::{Path, PathBuf};
1012

1113
pub struct TestOptions {
@@ -14,61 +16,87 @@ pub struct TestOptions {
1416
pub no_fail_fast: bool,
1517
}
1618

17-
pub fn run_tests(
18-
ws: &Workspace<'_>,
19-
options: &TestOptions,
20-
test_args: &[&str],
21-
) -> CargoResult<Option<CargoTestError>> {
19+
/// The kind of test.
20+
///
21+
/// This is needed because `Unit` does not track whether or not something is a
22+
/// benchmark.
23+
#[derive(Copy, Clone)]
24+
enum TestKind {
25+
Test,
26+
Bench,
27+
Doctest,
28+
}
29+
30+
/// A unit that failed to run.
31+
struct UnitTestError {
32+
unit: Unit,
33+
kind: TestKind,
34+
}
35+
36+
impl UnitTestError {
37+
/// Returns the CLI args needed to target this unit.
38+
fn cli_args(&self, ws: &Workspace<'_>, opts: &ops::CompileOptions) -> String {
39+
let mut args = if opts.spec.needs_spec_flag(ws) {
40+
format!("-p {} ", self.unit.pkg.name())
41+
} else {
42+
String::new()
43+
};
44+
let mut add = |which| write!(args, "--{which} {}", self.unit.target.name()).unwrap();
45+
46+
match self.kind {
47+
TestKind::Test | TestKind::Bench => match self.unit.target.kind() {
48+
TargetKind::Lib(_) => args.push_str("--lib"),
49+
TargetKind::Bin => add("bin"),
50+
TargetKind::Test => add("test"),
51+
TargetKind::Bench => add("bench"),
52+
TargetKind::ExampleLib(_) | TargetKind::ExampleBin => add("example"),
53+
TargetKind::CustomBuild => panic!("unexpected CustomBuild kind"),
54+
},
55+
TestKind::Doctest => args.push_str("--doc"),
56+
}
57+
args
58+
}
59+
}
60+
61+
/// Compiles and runs tests.
62+
///
63+
/// On error, the returned [`CliError`] will have the appropriate process exit
64+
/// code that Cargo should use.
65+
pub fn run_tests(ws: &Workspace<'_>, options: &TestOptions, test_args: &[&str]) -> CliResult {
2266
let compilation = compile_tests(ws, options)?;
2367

2468
if options.no_run {
2569
if !options.compile_opts.build_config.emit_json() {
2670
display_no_run_information(ws, test_args, &compilation, "unittests")?;
2771
}
28-
29-
return Ok(None);
72+
return Ok(());
3073
}
31-
let (test, mut errors) = run_unit_tests(ws.config(), options, test_args, &compilation)?;
74+
let mut errors = run_unit_tests(ws, options, test_args, &compilation, TestKind::Test)?;
3275

33-
// If we have an error and want to fail fast, then return.
34-
if !errors.is_empty() && !options.no_fail_fast {
35-
return Ok(Some(CargoTestError::new(test, errors)));
36-
}
37-
38-
let (doctest, docerrors) = run_doc_tests(ws, options, test_args, &compilation)?;
39-
let test = if docerrors.is_empty() { test } else { doctest };
40-
errors.extend(docerrors);
41-
if errors.is_empty() {
42-
Ok(None)
43-
} else {
44-
Ok(Some(CargoTestError::new(test, errors)))
45-
}
76+
let doctest_errors = run_doc_tests(ws, options, test_args, &compilation)?;
77+
errors.extend(doctest_errors);
78+
no_fail_fast_err(ws, &options.compile_opts, &errors)
4679
}
4780

48-
pub fn run_benches(
49-
ws: &Workspace<'_>,
50-
options: &TestOptions,
51-
args: &[&str],
52-
) -> CargoResult<Option<CargoTestError>> {
81+
/// Compiles and runs benchmarks.
82+
///
83+
/// On error, the returned [`CliError`] will have the appropriate process exit
84+
/// code that Cargo should use.
85+
pub fn run_benches(ws: &Workspace<'_>, options: &TestOptions, args: &[&str]) -> CliResult {
5386
let compilation = compile_tests(ws, options)?;
5487

5588
if options.no_run {
5689
if !options.compile_opts.build_config.emit_json() {
5790
display_no_run_information(ws, args, &compilation, "benches")?;
5891
}
59-
60-
return Ok(None);
92+
return Ok(());
6193
}
6294

6395
let mut args = args.to_vec();
6496
args.push("--bench");
6597

66-
let (test, errors) = run_unit_tests(ws.config(), options, &args, &compilation)?;
67-
68-
match errors.len() {
69-
0 => Ok(None),
70-
_ => Ok(Some(CargoTestError::new(test, errors))),
71-
}
98+
let errors = run_unit_tests(ws, options, &args, &compilation, TestKind::Bench)?;
99+
no_fail_fast_err(ws, &options.compile_opts, &errors)
72100
}
73101

74102
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
78106
}
79107

80108
/// Runs the unit and integration tests of a package.
109+
///
110+
/// Returns a `Vec` of tests that failed when `--no-fail-fast` is used.
111+
/// If `--no-fail-fast` is *not* used, then this returns an `Err`.
81112
fn run_unit_tests(
82-
config: &Config,
113+
ws: &Workspace<'_>,
83114
options: &TestOptions,
84115
test_args: &[&str],
85116
compilation: &Compilation<'_>,
86-
) -> CargoResult<(Test, Vec<ProcessError>)> {
117+
test_kind: TestKind,
118+
) -> Result<Vec<UnitTestError>, CliError> {
119+
let config = ws.config();
87120
let cwd = config.cwd();
88121
let mut errors = Vec::new();
89122

@@ -110,46 +143,32 @@ fn run_unit_tests(
110143
.shell()
111144
.verbose(|shell| shell.status("Running", &cmd))?;
112145

113-
let result = cmd.exec();
114-
115-
if let Err(e) = result {
116-
let e = e.downcast::<ProcessError>()?;
117-
errors.push((
118-
unit.target.kind().clone(),
119-
unit.target.name().to_string(),
120-
unit.pkg.name().to_string(),
121-
e,
122-
));
146+
if let Err(e) = cmd.exec() {
147+
let code = fail_fast_code(&e);
148+
let unit_err = UnitTestError {
149+
unit: unit.clone(),
150+
kind: test_kind,
151+
};
152+
report_test_error(ws, &options.compile_opts, &unit_err, e);
153+
errors.push(unit_err);
123154
if !options.no_fail_fast {
124-
break;
155+
return Err(CliError::code(code));
125156
}
126157
}
127158
}
128-
129-
if errors.len() == 1 {
130-
let (kind, name, pkg_name, e) = errors.pop().unwrap();
131-
Ok((
132-
Test::UnitTest {
133-
kind,
134-
name,
135-
pkg_name,
136-
},
137-
vec![e],
138-
))
139-
} else {
140-
Ok((
141-
Test::Multiple,
142-
errors.into_iter().map(|(_, _, _, e)| e).collect(),
143-
))
144-
}
159+
Ok(errors)
145160
}
146161

162+
/// Runs doc tests.
163+
///
164+
/// Returns a `Vec` of tests that failed when `--no-fail-fast` is used.
165+
/// If `--no-fail-fast` is *not* used, then this returns an `Err`.
147166
fn run_doc_tests(
148167
ws: &Workspace<'_>,
149168
options: &TestOptions,
150169
test_args: &[&str],
151170
compilation: &Compilation<'_>,
152-
) -> CargoResult<(Test, Vec<ProcessError>)> {
171+
) -> Result<Vec<UnitTestError>, CliError> {
153172
let config = ws.config();
154173
let mut errors = Vec::new();
155174
let doctest_xcompile = config.cli_unstable().doctest_xcompile;
@@ -258,16 +277,24 @@ fn run_doc_tests(
258277
.shell()
259278
.verbose(|shell| shell.status("Running", p.to_string()))?;
260279
if let Err(e) = p.exec() {
261-
let e = e.downcast::<ProcessError>()?;
262-
errors.push(e);
280+
let code = fail_fast_code(&e);
281+
let unit_err = UnitTestError {
282+
unit: unit.clone(),
283+
kind: TestKind::Doctest,
284+
};
285+
report_test_error(ws, &options.compile_opts, &unit_err, e);
286+
errors.push(unit_err);
263287
if !options.no_fail_fast {
264-
return Ok((Test::Doc, errors));
288+
return Err(CliError::code(code));
265289
}
266290
}
267291
}
268-
Ok((Test::Doc, errors))
292+
Ok(errors)
269293
}
270294

295+
/// Displays human-readable descriptions of the test executables.
296+
///
297+
/// This is used when `cargo test --no-run` is used.
271298
fn display_no_run_information(
272299
ws: &Workspace<'_>,
273300
test_args: &[&str],
@@ -303,6 +330,11 @@ fn display_no_run_information(
303330
return Ok(());
304331
}
305332

333+
/// Creates a [`ProcessBuilder`] for executing a single test.
334+
///
335+
/// Returns a tuple `(exe_display, process)` where `exe_display` is a string
336+
/// to display that describes the executable path in a human-readable form.
337+
/// `process` is the `ProcessBuilder` to use for executing the test.
306338
fn cmd_builds(
307339
config: &Config,
308340
cwd: &Path,
@@ -341,3 +373,67 @@ fn cmd_builds(
341373

342374
Ok((exe_display, cmd))
343375
}
376+
377+
/// Returns the error code to use when *not* using `--no-fail-fast`.
378+
///
379+
/// Cargo will return the error code from the test process itself. If some
380+
/// other error happened (like a failure to launch the process), then it will
381+
/// return a standard 101 error code.
382+
///
383+
/// When using `--no-fail-fast`, Cargo always uses the 101 exit code (since
384+
/// there may not be just one process to report).
385+
fn fail_fast_code(error: &anyhow::Error) -> i32 {
386+
if let Some(proc_err) = error.downcast_ref::<ProcessError>() {
387+
if let Some(code) = proc_err.code {
388+
return code;
389+
}
390+
}
391+
101
392+
}
393+
394+
/// Returns the `CliError` when using `--no-fail-fast` and there is at least
395+
/// one error.
396+
fn no_fail_fast_err(
397+
ws: &Workspace<'_>,
398+
opts: &ops::CompileOptions,
399+
errors: &[UnitTestError],
400+
) -> CliResult {
401+
// TODO: This could be improved by combining the flags on a single line when feasible.
402+
let args: Vec<_> = errors
403+
.iter()
404+
.map(|unit_err| format!(" `{}`", unit_err.cli_args(ws, opts)))
405+
.collect();
406+
let message = match errors.len() {
407+
0 => return Ok(()),
408+
1 => format!("1 target failed:\n{}", args.join("\n")),
409+
n => format!("{n} targets failed:\n{}", args.join("\n")),
410+
};
411+
Err(anyhow::Error::msg(message).into())
412+
}
413+
414+
/// Displays an error on the console about a test failure.
415+
fn report_test_error(
416+
ws: &Workspace<'_>,
417+
opts: &ops::CompileOptions,
418+
unit_err: &UnitTestError,
419+
test_error: anyhow::Error,
420+
) {
421+
let which = match unit_err.kind {
422+
TestKind::Test => "test failed",
423+
TestKind::Bench => "bench failed",
424+
TestKind::Doctest => "doctest failed",
425+
};
426+
427+
let mut err = format_err!("{}, to rerun pass `{}`", which, unit_err.cli_args(ws, opts));
428+
// Don't show "process didn't exit successfully" for simple errors.
429+
// libtest exits with 101 for normal errors.
430+
let is_simple = test_error
431+
.downcast_ref::<ProcessError>()
432+
.and_then(|proc_err| proc_err.code)
433+
.map_or(false, |code| code == 101);
434+
if !is_simple {
435+
err = test_error.context(err);
436+
}
437+
438+
crate::display_error(&err, &mut ws.config().shell());
439+
}

0 commit comments

Comments
 (0)