From 7eb4c44c0c654b7955b6fa1f6c9e0140a6c3d7a2 Mon Sep 17 00:00:00 2001 From: Aidan De Angelis Date: Sat, 30 Nov 2024 08:44:34 -0800 Subject: [PATCH] feat(max-fail): introduce --max-fail runner option --- cargo-nextest/src/dispatch.rs | 43 +++++++++++++++++--- nextest-runner/src/config/max_fail.rs | 37 ++++++++++++++++++ nextest-runner/src/config/mod.rs | 2 + nextest-runner/src/errors.rs | 16 ++++++++ nextest-runner/src/reporter/displayer.rs | 5 ++- nextest-runner/src/runner.rs | 50 +++++++++++++----------- site/src/docs/running.md | 5 ++- 7 files changed, 126 insertions(+), 32 deletions(-) create mode 100644 nextest-runner/src/config/max_fail.rs diff --git a/cargo-nextest/src/dispatch.rs b/cargo-nextest/src/dispatch.rs index 4f3bfb348b7..1df7eb7fae7 100644 --- a/cargo-nextest/src/dispatch.rs +++ b/cargo-nextest/src/dispatch.rs @@ -16,8 +16,9 @@ use nextest_metadata::BuildPlatform; use nextest_runner::{ cargo_config::{CargoConfigs, EnvironmentMap, TargetTriple}, config::{ - get_num_cpus, ConfigExperimental, EarlyProfile, NextestConfig, NextestVersionConfig, - NextestVersionEval, RetryPolicy, TestGroup, TestThreads, ToolConfigFile, VersionOnlyConfig, + get_num_cpus, ConfigExperimental, EarlyProfile, MaxFail, NextestConfig, + NextestVersionConfig, NextestVersionEval, RetryPolicy, TestGroup, TestThreads, + ToolConfigFile, VersionOnlyConfig, }, double_spawn::DoubleSpawnInfo, errors::WriteTestListError, @@ -845,9 +846,24 @@ pub struct TestRunnerOpts { fail_fast: bool, /// Run all tests regardless of failure - #[arg(long, conflicts_with = "no-run", overrides_with = "fail-fast")] + #[arg( + long, + name = "no-fail-fast", + conflicts_with = "no-run", + overrides_with = "fail-fast" + )] no_fail_fast: bool, + /// Number of tests that can fail before exiting test run [possible values: integer or "all"] + #[arg( + long, + name = "max-fail", + value_name = "N", + conflicts_with_all = &["no-run", "fail-fast", "no-fail-fast"], + require_equals = true, + )] + max_fail: Option, + /// Behavior if there are no tests to run [default: fail] #[arg( long, @@ -887,10 +903,17 @@ impl TestRunnerOpts { if let Some(retries) = self.retries { builder.set_retries(RetryPolicy::new_without_delay(retries)); } - if self.no_fail_fast { - builder.set_fail_fast(false); + if let Some(max_fail) = self.max_fail { + match max_fail { + MaxFail::Count(n) => { + builder.set_max_fail(n); + } + MaxFail::All => {} + } + } else if self.no_fail_fast { + // Ignore --fail-fast } else if self.fail_fast { - builder.set_fail_fast(true); + builder.set_max_fail(1); } if let Some(test_threads) = self.test_threads { builder.set_test_threads(test_threads); @@ -2420,6 +2443,7 @@ mod tests { "cargo nextest run --no-run --no-fail-fast", ArgumentConflict, ), + ("cargo nextest run --no-run --max-fail=3", ArgumentConflict), ( "cargo nextest run --no-run --failure-output immediate", ArgumentConflict, @@ -2437,6 +2461,13 @@ mod tests { ArgumentConflict, ), // --- + // --max-fail and these options conflict + // --- + ( + "cargo nextest run --max-fail=3 --no-fail-fast", + ArgumentConflict, + ), + // --- // Reuse build options conflict with cargo options // --- ( diff --git a/nextest-runner/src/config/max_fail.rs b/nextest-runner/src/config/max_fail.rs new file mode 100644 index 00000000000..e2c4e50762f --- /dev/null +++ b/nextest-runner/src/config/max_fail.rs @@ -0,0 +1,37 @@ +use crate::errors::MaxFailParseError; +use std::{fmt, str::FromStr}; + +/// Type for the max-fail flag +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum MaxFail { + /// Allow a specific number of tests to fail before exiting. + Count(usize), + + /// Run all tests. Equivalent to --no-fast-fail. + All, +} + +impl FromStr for MaxFail { + type Err = MaxFailParseError; + + fn from_str(s: &str) -> Result { + if s.to_lowercase() == "all" { + return Ok(Self::All); + } + + match s.parse::() { + Err(e) => Err(MaxFailParseError::new(format!("Error: {e} parsing {s}"))), + Ok(j) if j <= 0 => Err(MaxFailParseError::new("max-fail may not be <= 0")), + Ok(j) => Ok(MaxFail::Count(j as usize)), + } + } +} + +impl fmt::Display for MaxFail { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::All => write!(f, "all"), + Self::Count(n) => write!(f, "{n}"), + } + } +} diff --git a/nextest-runner/src/config/mod.rs b/nextest-runner/src/config/mod.rs index 85494c5f807..30679ae1b19 100644 --- a/nextest-runner/src/config/mod.rs +++ b/nextest-runner/src/config/mod.rs @@ -22,6 +22,7 @@ mod archive; mod config_impl; mod helpers; mod identifier; +mod max_fail; mod nextest_version; mod overrides; mod retry_policy; @@ -36,6 +37,7 @@ mod track_default; pub use archive::*; pub use config_impl::*; pub use identifier::*; +pub use max_fail::*; pub use nextest_version::*; pub use overrides::*; pub use retry_policy::*; diff --git a/nextest-runner/src/errors.rs b/nextest-runner/src/errors.rs index 33f0ecdb80d..12dcbb60403 100644 --- a/nextest-runner/src/errors.rs +++ b/nextest-runner/src/errors.rs @@ -554,6 +554,22 @@ pub enum ToolConfigFileParseError { }, } +/// Error returned while parsing a [`MaxFail`](crate::config::MaxFail) input. +#[derive(Clone, Debug, Error)] +#[error("unrecognized value for max-fail: {input}\n(hint: expected either an integer or \"all\")")] +pub struct MaxFailParseError { + /// The input that failed to parse. + pub input: String, +} + +impl MaxFailParseError { + pub(crate) fn new(input: impl Into) -> Self { + Self { + input: input.into(), + } + } +} + /// Error returned while parsing a [`TestThreads`](crate::config::TestThreads) value. #[derive(Clone, Debug, Error)] #[error( diff --git a/nextest-runner/src/reporter/displayer.rs b/nextest-runner/src/reporter/displayer.rs index 79b519211ae..526940737d1 100644 --- a/nextest-runner/src/reporter/displayer.rs +++ b/nextest-runner/src/reporter/displayer.rs @@ -1982,7 +1982,7 @@ fn write_final_warnings( if cancel_status == Some(CancelReason::TestFailure) { writeln!( writer, - "{}: {}/{} {} {} not run due to {} (run with {} to run all tests)", + "{}: {}/{} {} {} not run due to {} (run with {} to run all tests, or run with {})", "warning".style(styles.skip), not_run.style(styles.count), initial_run_count.style(styles.count), @@ -1990,6 +1990,7 @@ fn write_final_warnings( plural::were_plural_if(initial_run_count != 1 || not_run != 1), CancelReason::TestFailure.to_static_str().style(styles.skip), "--no-fail-fast".style(styles.count), + "--max-fail".style(styles.count), )?; } else { let due_to_reason = match cancel_status { @@ -2650,7 +2651,7 @@ mod tests { assert_eq!( warnings, "warning: 1/3 tests were not run due to test failure \ - (run with --no-fail-fast to run all tests)\n" + (run with --no-fail-fast to run all tests, or run with --max-fail)\n" ); let warnings = final_warnings_for( diff --git a/nextest-runner/src/runner.rs b/nextest-runner/src/runner.rs index a4baeb60c53..b660c72cbe6 100644 --- a/nextest-runner/src/runner.rs +++ b/nextest-runner/src/runner.rs @@ -132,7 +132,7 @@ impl Iterator for BackoffIter { pub struct TestRunnerBuilder { capture_strategy: CaptureStrategy, retries: Option, - fail_fast: Option, + max_fail: Option, test_threads: Option, } @@ -158,9 +158,9 @@ impl TestRunnerBuilder { self } - /// Sets the fail-fast value for this test runner. - pub fn set_fail_fast(&mut self, fail_fast: bool) -> &mut Self { - self.fail_fast = Some(fail_fast); + /// Sets the max-fail value for this test runner. + pub fn set_max_fail(&mut self, max_fail: usize) -> &mut Self { + self.max_fail = Some(max_fail); self } @@ -187,7 +187,7 @@ impl TestRunnerBuilder { .unwrap_or_else(|| profile.test_threads()) .compute(), }; - let fail_fast = self.fail_fast.unwrap_or_else(|| profile.fail_fast()); + let max_fail = self.max_fail.or_else(|| profile.fail_fast().then_some(1)); let runtime = Runtime::new().map_err(TestRunnerBuildError::TokioRuntimeCreate)?; let _guard = runtime.enter(); @@ -202,7 +202,7 @@ impl TestRunnerBuilder { cli_args, test_threads, force_retries: self.retries, - fail_fast, + max_fail, test_list, double_spawn, target_runner, @@ -308,7 +308,7 @@ struct TestRunnerInner<'a> { test_threads: usize, // This is Some if the user specifies a retry policy over the command-line. force_retries: Option, - fail_fast: bool, + max_fail: Option, test_list: &'a TestList<'a>, double_spawn: DoubleSpawnInfo, target_runner: TargetRunner, @@ -337,7 +337,7 @@ impl<'a> TestRunnerInner<'a> { self.profile.name(), self.cli_args.clone(), self.test_list.run_count(), - self.fail_fast, + self.max_fail, ); // Send the initial event. @@ -1618,26 +1618,28 @@ pub struct RunStats { impl RunStats { /// Returns true if there are any failures recorded in the stats. pub fn has_failures(&self) -> bool { - self.setup_scripts_failed > 0 - || self.setup_scripts_exec_failed > 0 - || self.setup_scripts_timed_out > 0 - || self.failed > 0 - || self.exec_failed > 0 - || self.timed_out > 0 + self.failed_setup_script_count() > 0 || self.failed_count() > 0 + } + + /// Returns count of setup scripts that did not pass. + pub fn failed_setup_script_count(&self) -> usize { + self.setup_scripts_failed + self.setup_scripts_exec_failed + self.setup_scripts_timed_out + } + + /// Returns count of tests that did not pass. + pub fn failed_count(&self) -> usize { + self.failed + self.exec_failed + self.timed_out } /// Summarizes the stats as an enum at the end of a test run. pub fn summarize_final(&self) -> FinalRunStats { // Check for failures first. The order of setup scripts vs tests should not be important, // though we don't assert that here. - if self.setup_scripts_failed > 0 - || self.setup_scripts_exec_failed > 0 - || self.setup_scripts_timed_out > 0 - { + if self.failed_setup_script_count() > 0 { FinalRunStats::Failed(RunStatsFailureKind::SetupScript) } else if self.setup_scripts_initial_count > self.setup_scripts_finished_count { FinalRunStats::Cancelled(RunStatsFailureKind::SetupScript) - } else if self.failed > 0 || self.exec_failed > 0 || self.timed_out > 0 { + } else if self.failed_count() > 0 { FinalRunStats::Failed(RunStatsFailureKind::Test { initial_run_count: self.initial_run_count, not_run: self.initial_run_count.saturating_sub(self.finished_count), @@ -1869,7 +1871,7 @@ struct CallbackContext<'a, F> { cli_args: Vec, stopwatch: StopwatchStart, run_stats: RunStats, - fail_fast: bool, + max_fail: Option, running_setup_script: Option>, running_tests: BTreeMap, ContextTestInstance<'a>>, cancel_state: Option, @@ -1886,7 +1888,7 @@ where profile_name: &str, cli_args: Vec, initial_run_count: usize, - fail_fast: bool, + max_fail: Option, ) -> Self { Self { callback, @@ -1898,7 +1900,7 @@ where initial_run_count, ..RunStats::default() }, - fail_fast, + max_fail, running_setup_script: None, running_tests: BTreeMap::new(), cancel_state: None, @@ -2078,7 +2080,9 @@ where self.run_stats.on_test_finished(&run_statuses); // should this run be cancelled because of a failure? - let fail_cancel = self.fail_fast && !run_statuses.last_status().result.is_success(); + let fail_cancel = self + .max_fail + .map_or(false, |mf| self.run_stats.failed_count() >= mf); self.callback(TestEventKind::TestFinished { test_instance, diff --git a/site/src/docs/running.md b/site/src/docs/running.md index bf16291a797..cddac6446a2 100644 --- a/site/src/docs/running.md +++ b/site/src/docs/running.md @@ -184,8 +184,11 @@ cargo nextest run -E 'platform(host)' ## Other runner options +`--max-fail=N` +: Number of tests that can fail before aborting the test run. Useful for uncovering multiple issues without having to run the whole test suite. + `--no-fail-fast` -: Do not exit the test run on the first failure. Most useful for CI scenarios. +: Do not exit the test run on the first failure. Most useful for CI scenarios. Equivalent to `--max-fail=all` `-j`, `--test-threads` : Number of tests to run simultaneously. Note that this is separate from the number of build jobs to run simultaneously, which is specified by `--build-jobs`.