From b49e2e067561d2bcdb57e7fa01a2ce9ff084266a Mon Sep 17 00:00:00 2001 From: Rain Date: Sun, 18 Aug 2024 19:39:57 -0700 Subject: [PATCH] [cargo-nextest] add a new --no-tests option Default to `warn` currently, but can also be `fail` or `pass`. The plan is to default to `fail` in the future (see #1646). --- cargo-nextest/src/dispatch.rs | 60 +++++++++++++-- cargo-nextest/src/errors.rs | 16 ++++ integration-tests/tests/integration/main.rs | 81 +++++++++++++++++++++ nextest-metadata/src/exit_codes.rs | 8 ++ nextest-runner/src/reporter.rs | 42 +++++------ nextest-runner/src/runner.rs | 2 +- 6 files changed, 178 insertions(+), 31 deletions(-) diff --git a/cargo-nextest/src/dispatch.rs b/cargo-nextest/src/dispatch.rs index c9742399846..d8d911b6485 100644 --- a/cargo-nextest/src/dispatch.rs +++ b/cargo-nextest/src/dispatch.rs @@ -11,6 +11,7 @@ use camino::{Utf8Path, Utf8PathBuf}; use clap::{builder::BoolishValueParser, ArgAction, Args, Parser, Subcommand, ValueEnum}; use guppy::graph::PackageGraph; use itertools::Itertools; +use log::warn; use nextest_filtering::FilteringExpr; use nextest_metadata::BuildPlatform; use nextest_runner::{ @@ -406,8 +407,7 @@ impl NtrOpts { &self.run_opts.reporter_opts, cli_args, output_writer, - )?; - Ok(0) + ) } } @@ -754,6 +754,33 @@ pub struct TestRunnerOpts { /// Run all tests regardless of failure #[arg(long, conflicts_with = "no-run", overrides_with = "fail-fast")] no_fail_fast: bool, + + /// Behavior if there are no tests to run. + /// + /// The default is currently `warn`, but it will change to `fail` in the future. + #[arg( + long, + value_enum, + conflicts_with = "no-run", + value_name = "ACTION", + require_equals = true, + env = "NEXTEST_NO_TESTS" + )] + no_tests: Option, +} + +#[derive(Clone, Copy, Debug, Default, ValueEnum)] +enum NoTestsBehavior { + /// Silently exit with code 0. + Pass, + + /// Produce a warning and exit with code 0. + #[default] + Warn, + + /// Produce an error message and exit with code 4. + #[clap(alias = "error")] + Fail, } impl TestRunnerOpts { @@ -1563,7 +1590,7 @@ impl App { reporter_opts: &TestReporterOpts, cli_args: Vec, output_writer: &mut OutputWriter, - ) -> Result<()> { + ) -> Result { let (version_only_config, config) = self.base.load_config()?; let profile = self.base.load_profile(profile_name, &config)?; @@ -1638,7 +1665,7 @@ impl App { Some(runner_builder) => runner_builder, None => { // This means --no-run was passed in. Exit. - return Ok(()); + return Ok(0); } }; @@ -1656,15 +1683,32 @@ impl App { // Write and flush the event. reporter.report_event(event) })?; + reporter.finish(); self.base .check_version_config_final(version_only_config.nextest_version())?; match run_stats.summarize_final() { - FinalRunStats::Success => Ok(()), + FinalRunStats::Success => Ok(0), FinalRunStats::NoTestsRun => { - // This currently does not exit with a non-zero code, but will in the future: - // https://github.com/nextest-rs/nextest/issues/1639 - Ok(()) + match runner_opts.no_tests { + Some(NoTestsBehavior::Pass) => Ok(0), + Some(NoTestsBehavior::Warn) => { + warn!("no tests to run"); + Ok(0) + } + Some(NoTestsBehavior::Fail) => { + Err(ExpectedError::NoTestsRun { is_default: false }) + } + None => { + // This currently does not exit with a non-zero code, but will in the + // future: https://github.com/nextest-rs/nextest/issues/1639 + warn!( + "no tests to run -- this will become an error in the future\n\ + (hint: use `--no-tests` to customize)" + ); + Ok(0) + } + } } FinalRunStats::Canceled(RunStatsFailureKind::SetupScript) | FinalRunStats::Failed(RunStatsFailureKind::SetupScript) => { diff --git a/cargo-nextest/src/errors.rs b/cargo-nextest/src/errors.rs index 362d4bb3513..d00668c1cc9 100644 --- a/cargo-nextest/src/errors.rs +++ b/cargo-nextest/src/errors.rs @@ -184,6 +184,12 @@ pub enum ExpectedError { SetupScriptFailed, #[error("test run failed")] TestRunFailed, + #[error("no tests to run")] + NoTestsRun { + /// The no-tests-run error was chosen because it was the default (we show a hint in this + /// case) + is_default: bool, + }, #[cfg(feature = "self-update")] #[error("failed to parse --version")] UpdateVersionParseError { @@ -406,6 +412,7 @@ impl ExpectedError { } Self::SetupScriptFailed => NextestExitCode::SETUP_SCRIPT_FAILED, Self::TestRunFailed => NextestExitCode::TEST_RUN_FAILED, + Self::NoTestsRun { .. } => NextestExitCode::NO_TESTS_RUN, Self::ArchiveCreateError { .. } => NextestExitCode::ARCHIVE_CREATION_FAILED, Self::WriteTestListError { .. } | Self::WriteEventError { .. } => { NextestExitCode::WRITE_OUTPUT_ERROR @@ -731,6 +738,15 @@ impl ExpectedError { log::error!("test run failed"); None } + Self::NoTestsRun { is_default } => { + let hint_str = if *is_default { + "\n(hint: use `--no-tests` to customize)" + } else { + "" + }; + log::error!("no tests to run{hint_str}"); + None + } Self::ShowTestGroupsError { err } => { log::error!("{err}"); err.source() diff --git a/integration-tests/tests/integration/main.rs b/integration-tests/tests/integration/main.rs index f284a3cf38b..c49bfd522ef 100644 --- a/integration-tests/tests/integration/main.rs +++ b/integration-tests/tests/integration/main.rs @@ -228,6 +228,87 @@ fn test_list_target_after_build() { check_list_full_output(&output.stdout, Some(BuildPlatform::Target)); } +#[test] +fn test_run_no_tests() { + set_env_vars(); + + let p = TempProject::new().unwrap(); + build_tests(&p); + + let output = CargoNextestCli::new() + .args([ + "--manifest-path", + p.manifest_path().as_str(), + "run", + "-E", + "none()", + ]) + .output(); + + let stderr = output.stderr_as_str(); + assert!( + stderr.contains("warning: no tests to run -- this will become an error in the future"), + "stderr contains no tests message" + ); + + let output = CargoNextestCli::new() + .args([ + "--manifest-path", + p.manifest_path().as_str(), + "run", + "-E", + "none()", + "--no-tests=warn", + ]) + .output(); + + let stderr = output.stderr_as_str(); + assert!( + stderr.contains("warning: no tests to run"), + "stderr contains no tests message" + ); + + let output = CargoNextestCli::new() + .args([ + "--manifest-path", + p.manifest_path().as_str(), + "run", + "-E", + "none()", + "--no-tests=fail", + ]) + .unchecked(true) + .output(); + assert_eq!( + output.exit_status.code(), + Some(NextestExitCode::NO_TESTS_RUN), + "correct exit code for command\n{output}" + ); + + let stderr = output.stderr_as_str(); + assert!( + stderr.contains("error: no tests to run"), + "stderr contains no tests message: {output}" + ); + + let output = CargoNextestCli::new() + .args([ + "--manifest-path", + p.manifest_path().as_str(), + "run", + "-E", + "none()", + "--no-tests=pass", + ]) + .output(); + + let stderr = output.stderr_as_str(); + assert!( + !stderr.contains("no tests to run"), + "no tests message does not error out" + ); +} + #[test] fn test_run() { set_env_vars(); diff --git a/nextest-metadata/src/exit_codes.rs b/nextest-metadata/src/exit_codes.rs index 5514589c75f..776abacea2b 100644 --- a/nextest-metadata/src/exit_codes.rs +++ b/nextest-metadata/src/exit_codes.rs @@ -19,6 +19,14 @@ impl NextestExitCode { /// An error was encountered while attempting to double-spawn a nextest process. pub const DOUBLE_SPAWN_ERROR: i32 = 70; + /// No tests were selected to run, but no other errors occurred. + /// + /// This is an advisory exit code generated if nextest is run with `--no-tests=fail` (soon to + /// become the default). See [discussion #1646] for more. + /// + /// [discussion #1646]: https://github.com/nextest-rs/nextest/discussions/1646 + pub const NO_TESTS_RUN: i32 = 4; + /// One or more tests failed. pub const TEST_RUN_FAILED: i32 = 100; diff --git a/nextest-runner/src/reporter.rs b/nextest-runner/src/reporter.rs index 457ea508be3..5102e031370 100644 --- a/nextest-runner/src/reporter.rs +++ b/nextest-runner/src/reporter.rs @@ -342,6 +342,17 @@ enum ReporterStderrImpl<'a> { Buffer(&'a mut Vec), } +impl ReporterStderrImpl<'_> { + fn finish_and_clear_bar(&self) { + match self { + ReporterStderrImpl::TerminalWithBar(bar) => { + bar.finish_and_clear(); + } + ReporterStderrImpl::TerminalWithoutBar | ReporterStderrImpl::Buffer(_) => {} + } + } +} + /// Functionality to report test results to stderr, JUnit, and/or structured, /// machine-readable results to stdout pub struct TestReporter<'a> { @@ -364,6 +375,11 @@ impl<'a> TestReporter<'a> { self.write_event(event) } + /// Mark the reporter done. + pub fn finish(&mut self) { + self.stderr.finish_and_clear_bar(); + } + // --- // Helper methods // --- @@ -1021,8 +1037,7 @@ impl<'a> TestReporterImpl<'a> { } } - // Print out warnings at the end, if any. We currently print out warnings in two - // cases: + // Print out warnings at the end, if any. write_final_warnings(stats_summary, self.cancel_status, &self.styles, writer)?; } } @@ -1634,7 +1649,6 @@ fn write_final_warnings( writer: &mut dyn Write, ) -> io::Result<()> { match final_stats { - // 1. When some tests are not run due to a test failure. FinalRunStats::Failed(RunStatsFailureKind::Test { initial_run_count, not_run, @@ -1674,16 +1688,6 @@ fn write_final_warnings( )?; } } - - // 2. When no tests are run at all. - FinalRunStats::NoTestsRun => { - writeln!( - writer, - "{}: no tests were run (this will exit with a \ - non-zero code in the future)", - "warning".style(styles.skip) - )?; - } _ => {} } @@ -2406,17 +2410,11 @@ mod tests { ); assert_eq!(warnings, "warning: 1/1 test was not run due to interrupt\n"); + // These warnings are taken care of by cargo-nextest. let warnings = final_warnings_for(FinalRunStats::NoTestsRun, None); - assert_eq!( - warnings, - "warning: no tests were run (this will exit with a non-zero code in the future)\n" - ); - + assert_eq!(warnings, ""); let warnings = final_warnings_for(FinalRunStats::NoTestsRun, Some(CancelReason::Signal)); - assert_eq!( - warnings, - "warning: no tests were run (this will exit with a non-zero code in the future)\n" - ); + assert_eq!(warnings, ""); // No warnings for success. let warnings = final_warnings_for(FinalRunStats::Success, None); diff --git a/nextest-runner/src/runner.rs b/nextest-runner/src/runner.rs index a03b3d1cbec..30a86468386 100644 --- a/nextest-runner/src/runner.rs +++ b/nextest-runner/src/runner.rs @@ -1649,7 +1649,7 @@ pub enum FinalRunStats { /// The test run was successful, or is successful so far. Success, - /// The test run was successful, or is successful so far, but no tests were run. + /// The test run was successful, or is successful so far, but no tests were selected to run. NoTestsRun, /// The test run was canceled.