Check nextest error codes (sourcefrog#275)

Fixes sourcefrog#238

Also log errors/signals that killed subprocesses

Cargo.toml

@@ -54,6 +54,7 @@ ignore = "0.4.20"
 indoc = "2.0.0"
 itertools = "0.11"
 mutants = "0.0.3"
+nextest-metadata = "0.10"
 nix = "0.27"
 patch = "0.7"
 path-slash = "0.2"

NEWS.md

@@ -6,6 +6,10 @@
 
 - Changed: Minimum Rust version (to build cargo-mutants, not to use it) increased to 1.73.
 
+- New: Warn if nextest returns an exit code indicating some failure other than test failure, such as an internal error in nextest.
+
+- New: json output includes the exit code of subprocesses, and the signal if it was killed by a signal.
+
 ## 24.2.0
 
 - New: Colored output can be enabled in CI or other noninteractive situations by passing `--colors=always`, or setting `CARGO_TERM_COLOR=always`, or `CLICOLOR_FORCE=1`. Colors can similarly be forced off with `--colors=never`, `CARGO_TERM_COLOR=never`, or `NO_COLOR=1`.

src/cargo.rs

@@ -8,12 +8,13 @@ use std::time::{Duration, Instant};
 use anyhow::Result;
 use camino::Utf8Path;
 use itertools::Itertools;
-use tracing::{debug, debug_span};
+use nextest_metadata::NextestExitCode;
+use tracing::{debug, debug_span, warn};
 
 use crate::options::TestTool;
 use crate::outcome::PhaseResult;
 use crate::package::Package;
-use crate::process::Process;
+use crate::process::{Process, ProcessStatus};
 use crate::*;
 
 /// Run cargo build, check, or test.
@@ -39,6 +40,17 @@ pub fn run_cargo(
     let process_status = Process::run(&argv, &env, build_dir.path(), timeout, log_file, console)?;
     check_interrupted()?;
     debug!(?process_status, elapsed = ?start.elapsed());
+    if options.test_tool == TestTool::Nextest && phase == Phase::Test {
+        // Nextest returns detailed exit codes. I think we should still treat any non-zero result as just an
+        // error, but we can at least warn if it's unexpected.
+        if let ProcessStatus::Failure(code) = process_status {
+            // TODO: When we build with `nextest test --no-test` then we should also check build
+            // processes.
+            if code != NextestExitCode::TEST_RUN_FAILED as u32 {
+                warn!(%code, "nextest process exited with unexpected code (not TEST_RUN_FAILED)");
+            }
+        }
+    }
     Ok(PhaseResult {
         phase,
         duration: start.elapsed(),

src/lab.rs

@@ -240,7 +240,7 @@ fn test_scenario(
             options,
             console,
         )?;
-        let success = phase_result.is_success();
+        let success = phase_result.is_success(); // so we can move it away
         outcome.add_phase_result(phase_result);
         console.scenario_phase_finished(scenario, phase);
         if (phase == Phase::Check && options.check_only) || !success {

src/outcome.rs

@@ -1,4 +1,4 @@
-// Copyright 2022-2023 Martin Pool
+// Copyright 2022-2024 Martin Pool
 
 //! The outcome of running a single mutation scenario, or a whole lab.
 
@@ -216,33 +216,33 @@ impl ScenarioOutcome {
     }
 
     pub fn success(&self) -> bool {
-        self.last_phase_result().success()
+        self.last_phase_result().is_success()
     }
 
     pub fn has_timeout(&self) -> bool {
         self.phase_results
             .iter()
-            .any(|pr| pr.process_status.timeout())
+            .any(|pr| pr.process_status.is_timeout())
     }
 
     pub fn check_or_build_failed(&self) -> bool {
         self.phase_results
             .iter()
-            .any(|pr| pr.phase != Phase::Test && pr.process_status == ProcessStatus::Failure)
+            .any(|pr| pr.phase != Phase::Test && pr.process_status.is_failure())
     }
 
     /// True if this outcome is a caught mutant: it's a mutant and the tests failed.
     pub fn mutant_caught(&self) -> bool {
         self.scenario.is_mutant()
             && self.last_phase() == Phase::Test
-            && self.last_phase_result() == ProcessStatus::Failure
+            && self.last_phase_result().is_failure()
     }
 
     /// True if this outcome is a missed mutant: it's a mutant and the tests succeeded.
     pub fn mutant_missed(&self) -> bool {
         self.scenario.is_mutant()
             && self.last_phase() == Phase::Test
-            && self.last_phase_result().success()
+            && self.last_phase_result().is_success()
     }
 
     pub fn summary(&self) -> SummaryOutcome {
@@ -290,7 +290,7 @@ pub struct PhaseResult {
 
 impl PhaseResult {
     pub fn is_success(&self) -> bool {
-        self.process_status.success()
+        self.process_status.is_success()
     }
 }
 

src/process.rs

@@ -16,9 +16,8 @@ use std::time::{Duration, Instant};
 use anyhow::{anyhow, Context};
 use camino::Utf8Path;
 use serde::Serialize;
-use subprocess::{Popen, PopenConfig, Redirection};
-#[allow(unused_imports)]
-use tracing::{debug, debug_span, error, info, span, trace, warn, Level};
+use subprocess::{ExitStatus, Popen, PopenConfig, Redirection};
+use tracing::{debug, debug_span, error, span, trace, warn, Level};
 
 use crate::console::Console;
 use crate::interrupt::check_interrupted;
@@ -97,6 +96,7 @@ impl Process {
         })
     }
 
+    /// Check if the child process has finished; if so, return its status.
     #[mutants::skip] // It's hard to avoid timeouts if this never works...
     pub fn poll(&mut self) -> Result<Option<ProcessStatus>> {
         let elapsed = self.start.elapsed();
@@ -109,10 +109,11 @@ impl Process {
             self.terminate()?;
             Err(e)
         } else if let Some(status) = self.child.poll() {
-            if status.success() {
-                Ok(Some(ProcessStatus::Success))
-            } else {
-                Ok(Some(ProcessStatus::Failure))
+            match status {
+                _ if status.success() => Ok(Some(ProcessStatus::Success)),
+                ExitStatus::Exited(code) => Ok(Some(ProcessStatus::Failure(code))),
+                ExitStatus::Signaled(signal) => Ok(Some(ProcessStatus::Signalled(signal))),
+                ExitStatus::Undetermined | ExitStatus::Other(_) => Ok(Some(ProcessStatus::Other)),
             }
         } else {
             Ok(None)
@@ -189,19 +190,30 @@ fn terminate_child_impl(child: &mut Popen) -> Result<()> {
 /// The result of running a single child process.
 #[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize)]
 pub enum ProcessStatus {
+    /// Exited with status 0.
     Success,
-    Failure,
+    /// Exited with status non-0.
+    Failure(u32),
+    /// Exceeded its timeout, and killed.
     Timeout,
+    /// Killed by some signal.
+    Signalled(u8),
+    /// Unknown or unexpected situation.
+    Other,
 }
 
 impl ProcessStatus {
-    pub fn success(&self) -> bool {
+    pub fn is_success(&self) -> bool {
         *self == ProcessStatus::Success
     }
 
-    pub fn timeout(&self) -> bool {
+    pub fn is_timeout(&self) -> bool {
         *self == ProcessStatus::Timeout
     }
+
+    pub fn is_failure(&self) -> bool {
+        matches!(self, ProcessStatus::Failure(_))
+    }
 }
 
 #[cfg(unix)]

src/snapshots/cargo_mutants__visit__test__expected_mutants_for_own_source_tree.snap

@@ -11,6 +11,10 @@ src/build_dir.rs: replace BuildDir::in_place -> Result<BuildDir> with Err(::anyh
 src/build_dir.rs: replace BuildDir::path -> &Utf8Path with &Default::default()
 src/cargo.rs: replace run_cargo -> Result<PhaseResult> with Ok(Default::default())
 src/cargo.rs: replace run_cargo -> Result<PhaseResult> with Err(::anyhow::anyhow!("mutated!"))
+src/cargo.rs: replace && with || in run_cargo
+src/cargo.rs: replace == with != in run_cargo
+src/cargo.rs: replace == with != in run_cargo
+src/cargo.rs: replace != with == in run_cargo
 src/cargo.rs: replace cargo_bin -> String with String::new()
 src/cargo.rs: replace cargo_bin -> String with "xyzzy".into()
 src/cargo.rs: replace cargo_argv -> Vec<String> with vec![]
@@ -284,13 +288,11 @@ src/outcome.rs: replace ScenarioOutcome::check_or_build_failed -> bool with true
 src/outcome.rs: replace ScenarioOutcome::check_or_build_failed -> bool with false
 src/outcome.rs: replace && with || in ScenarioOutcome::check_or_build_failed
 src/outcome.rs: replace != with == in ScenarioOutcome::check_or_build_failed
-src/outcome.rs: replace == with != in ScenarioOutcome::check_or_build_failed
 src/outcome.rs: replace ScenarioOutcome::mutant_caught -> bool with true
 src/outcome.rs: replace ScenarioOutcome::mutant_caught -> bool with false
 src/outcome.rs: replace && with || in ScenarioOutcome::mutant_caught
 src/outcome.rs: replace && with || in ScenarioOutcome::mutant_caught
 src/outcome.rs: replace == with != in ScenarioOutcome::mutant_caught
-src/outcome.rs: replace == with != in ScenarioOutcome::mutant_caught
 src/outcome.rs: replace ScenarioOutcome::mutant_missed -> bool with true
 src/outcome.rs: replace ScenarioOutcome::mutant_missed -> bool with false
 src/outcome.rs: replace && with || in ScenarioOutcome::mutant_missed
@@ -351,12 +353,14 @@ src/process.rs: replace Process::terminate -> Result<()> with Err(::anyhow::anyh
 src/process.rs: replace terminate_child_impl -> Result<()> with Ok(())
 src/process.rs: replace terminate_child_impl -> Result<()> with Err(::anyhow::anyhow!("mutated!"))
 src/process.rs: replace != with == in terminate_child_impl
-src/process.rs: replace ProcessStatus::success -> bool with true
-src/process.rs: replace ProcessStatus::success -> bool with false
-src/process.rs: replace == with != in ProcessStatus::success
-src/process.rs: replace ProcessStatus::timeout -> bool with true
-src/process.rs: replace ProcessStatus::timeout -> bool with false
-src/process.rs: replace == with != in ProcessStatus::timeout
+src/process.rs: replace ProcessStatus::is_success -> bool with true
+src/process.rs: replace ProcessStatus::is_success -> bool with false
+src/process.rs: replace == with != in ProcessStatus::is_success
+src/process.rs: replace ProcessStatus::is_timeout -> bool with true
+src/process.rs: replace ProcessStatus::is_timeout -> bool with false
+src/process.rs: replace == with != in ProcessStatus::is_timeout
+src/process.rs: replace ProcessStatus::is_failure -> bool with true
+src/process.rs: replace ProcessStatus::is_failure -> bool with false
 src/process.rs: replace setpgid_on_unix -> PopenConfig with Default::default()
 src/process.rs: replace get_command_output -> Result<String> with Ok(String::new())
 src/process.rs: replace get_command_output -> Result<String> with Ok("xyzzy".into())

tests/cli/workspace.rs

@@ -160,7 +160,7 @@ fn workspace_tree_is_well_tested() {
             mutant_phases[0]["argv"].as_array().unwrap()[1..=3],
             ["build", "--tests", "--manifest-path"]
         );
-        assert_eq!(mutant_phases[1]["process_status"], "Failure");
+        assert_eq!(mutant_phases[1]["process_status"], json!({"Failure": 101}));
         assert_eq!(
             mutant_phases[1]["argv"].as_array().unwrap()[1..=2],
             ["test", "--manifest-path"],