From 63209515a3be395739e7f34bda4910e71c849803 Mon Sep 17 00:00:00 2001 From: David Peter Date: Wed, 7 Oct 2026 10:20:07 +0200 Subject: [PATCH] Improve command and hook failure diagnostics --- src/benchmark/executor.rs | 44 +++++++++++++------------ src/benchmark/mod.rs | 38 +++++++++++----------- tests/integration_tests.rs | 66 +++++++++++++++++++++++++++----------- 3 files changed, 89 insertions(+), 59 deletions(-) diff --git a/src/benchmark/executor.rs b/src/benchmark/executor.rs index a0e584c..5c18861 100644 --- a/src/benchmark/executor.rs +++ b/src/benchmark/executor.rs @@ -5,9 +5,7 @@ use std::process::ExitStatus; use crate::benchmark::measurement::Measurement; use crate::benchmark::measurement::Measurements; use crate::command::Command; -use crate::options::{ - CmdFailureAction, CommandInputPolicy, CommandOutputPolicy, Options, OutputStyleOption, Shell, -}; +use crate::options::{CmdFailureAction, CommandOutputPolicy, Options, OutputStyleOption, Shell}; use crate::output::progress_bar::get_progress_bar; use crate::quantity::{second, Information, Time, Zero}; use crate::timer::execute_and_measure; @@ -58,13 +56,13 @@ pub trait Executor { fn run_command_and_measure_common( mut command: std::process::Command, iteration: BenchmarkIteration, - command_failure_action: CmdFailureAction, - command_input_policy: &CommandInputPolicy, + command_failure_action: Option, + options: &Options, command_output_policy: &CommandOutputPolicy, command_name: &str, collect_hardware_counters: bool, ) -> Result { - let stdin = command_input_policy.get_stdin()?; + let stdin = options.command_input_policy.get_stdin()?; let (stdout, stderr) = command_output_policy.get_stdout_stderr()?; command.stdin(stdin).stdout(stdout).stderr(stderr); @@ -83,10 +81,14 @@ fn run_command_and_measure_common( if !measurement.exit_status.success() { use crate::util::exit_code::extract_exit_code; - let should_fail = match command_failure_action { + let show_ignore_failure_hint = command_failure_action.is_none(); + let should_fail = match command_failure_action + .as_ref() + .unwrap_or(&options.command_failure_action) + { CmdFailureAction::RaiseError => true, CmdFailureAction::IgnoreAllFailures => false, - CmdFailureAction::IgnoreSpecificFailures(ref codes) => { + CmdFailureAction::IgnoreSpecificFailures(codes) => { // Only fail if the exit code is not in the list of codes to ignore if let Some(exit_code) = extract_exit_code(measurement.exit_status) { !codes.contains(&exit_code) @@ -105,15 +107,17 @@ fn run_command_and_measure_common( BenchmarkIteration::Benchmark(0) => "the first benchmark run".to_string(), BenchmarkIteration::Benchmark(i) => format!("benchmark iteration {i}"), }; - bail!( - "{cause} in {when}. Use the '-i'/'--ignore-failure' option if you want to ignore this. \ - Alternatively, use the '--show-output' option to debug what went wrong.", - cause=measurement.exit_status.code().map_or( - "The process has been terminated by a signal".into(), - |c| format!("Command terminated with non-zero exit code {c}") - - ), + let cause = measurement.exit_status.code().map_or_else( + || "was terminated by a signal".to_owned(), + |code| format!("terminated with non-zero exit code {code}"), ); + let hint = if show_ignore_failure_hint { + " Use the '-i'/'--ignore-failure' option if you want to ignore this. \ + Alternatively, use the '--show-output' option to debug what went wrong." + } else { + " Use the '--show-output' option to debug what went wrong." + }; + bail!("Command '{command_name}' {cause} in {when}.{hint}"); } } @@ -141,8 +145,8 @@ impl Executor for RawExecutor<'_> { run_command_and_measure_common( command.get_command()?, iteration, - command_failure_action.unwrap_or_else(|| self.options.command_failure_action.clone()), - &self.options.command_input_policy, + command_failure_action, + self.options, output_policy, &command.get_command_line(), matches!(iteration, BenchmarkIteration::Benchmark(_)), @@ -197,8 +201,8 @@ impl Executor for ShellExecutor<'_> { let mut measurement = run_command_and_measure_common( command_builder, iteration, - command_failure_action.unwrap_or_else(|| self.options.command_failure_action.clone()), - &self.options.command_input_policy, + command_failure_action, + self.options, output_policy, &command.get_command_line(), false, diff --git a/src/benchmark/mod.rs b/src/benchmark/mod.rs index d0dce9d..2d28fd6 100644 --- a/src/benchmark/mod.rs +++ b/src/benchmark/mod.rs @@ -25,7 +25,7 @@ use crate::parameter::ParameterNameAndValue; use crate::quantity::{self, const_time_from_seconds, Time, Zero}; use benchmark_result::BenchmarkResult; -use anyhow::{anyhow, ensure, Result}; +use anyhow::{ensure, Context, Result}; use colored::*; use self::executor::Executor; @@ -55,11 +55,11 @@ impl<'a> Benchmark<'a> { } } - /// Run setup, cleanup, or preparation commands + /// Run setup, cleanup, preparation, or conclusion commands fn run_intermediate_command( &self, command: &Command<'_>, - error_output: &'static str, + kind: &'static str, output_policy: &CommandOutputPolicy, iteration: executor::BenchmarkIteration, ) -> Result { @@ -70,7 +70,17 @@ impl<'a> Benchmark<'a> { Some(CmdFailureAction::RaiseError), output_policy, ) - .map_err(|_| anyhow!(error_output)) + .with_context(|| { + let ignore_failure = if cfg!(windows) { + " || exit /b 0" + } else { + " || true" + }; + format!( + "The {kind} command failed. If this failure can be ignored, \ + use '-S'/'--shell=default' and append '{ignore_failure}' to the command" + ) + }) } /// Run the command specified by `--setup`. @@ -85,14 +95,11 @@ impl<'a> Benchmark<'a> { .as_ref() .map(|setup_command| Command::new_parametrized(None, setup_command, parameters)); - let error_output = "The setup command terminated with a non-zero exit code. \ - Append ' || true' to the command if you are sure that this can be ignored."; - Ok(command .map(|cmd| { self.run_intermediate_command( &cmd, - error_output, + "setup", output_policy, BenchmarkIteration::NonBenchmarkRun, ) @@ -113,14 +120,11 @@ impl<'a> Benchmark<'a> { .as_ref() .map(|cleanup_command| Command::new_parametrized(None, cleanup_command, parameters)); - let error_output = "The cleanup command terminated with a non-zero exit code. \ - Append ' || true' to the command if you are sure that this can be ignored."; - Ok(command .map(|cmd| { self.run_intermediate_command( &cmd, - error_output, + "cleanup", output_policy, BenchmarkIteration::NonBenchmarkRun, ) @@ -136,10 +140,7 @@ impl<'a> Benchmark<'a> { output_policy: &CommandOutputPolicy, iteration: executor::BenchmarkIteration, ) -> Result { - let error_output = "The preparation command terminated with a non-zero exit code. \ - Append ' || true' to the command if you are sure that this can be ignored."; - - self.run_intermediate_command(command, error_output, output_policy, iteration) + self.run_intermediate_command(command, "preparation", output_policy, iteration) } /// Run the command specified by `--conclude`. @@ -149,10 +150,7 @@ impl<'a> Benchmark<'a> { output_policy: &CommandOutputPolicy, iteration: executor::BenchmarkIteration, ) -> Result { - let error_output = "The conclusion command terminated with a non-zero exit code. \ - Append ' || true' to the command if you are sure that this can be ignored."; - - self.run_intermediate_command(command, error_output, output_policy, iteration) + self.run_intermediate_command(command, "conclusion", output_policy, iteration) } fn validate_measurement(&self, measurement: &Measurement) -> Result<()> { diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs index 2950d50..73feabd 100644 --- a/tests/integration_tests.rs +++ b/tests/integration_tests.rs @@ -275,7 +275,7 @@ fn fails_for_unknown_command() { .assert() .failure() .stderr(predicate::str::contains( - "Command terminated with non-zero exit code", + "Command 'some-nonexisting-program-b5d9574198b7e4b12a71fa4747c0a577' terminated with non-zero exit code", )); } @@ -298,12 +298,15 @@ fn fails_for_failing_command_without_shell() { hyperfine() .arg("--shell=none") .arg("--runs=1") - .arg("false") + .arg("--command-name=named benchmark") + .args(["--parameter-list", "status", "0,1"]) + .arg("test {status} -eq 0") .assert() .failure() .stderr(predicate::str::contains( - "Command terminated with non-zero exit code", - )); + "Command 'test 1 -eq 0' terminated with non-zero exit code 1 in the first benchmark run.", + )) + .stderr(predicate::str::contains("--ignore-failure")); } #[test] @@ -315,9 +318,7 @@ fn fails_for_unknown_setup_command() { .arg("echo test") .assert() .failure() - .stderr(predicate::str::contains( - "The setup command terminated with a non-zero exit code.", - )); + .stderr(predicate::str::contains("The setup command failed.")); } #[test] @@ -329,9 +330,7 @@ fn fails_for_unknown_cleanup_command() { .arg("echo test") .assert() .failure() - .stderr(predicate::str::contains( - "The cleanup command terminated with a non-zero exit code.", - )); + .stderr(predicate::str::contains("The cleanup command failed.")); } #[test] @@ -342,9 +341,7 @@ fn fails_for_unknown_prepare_command() { .arg("echo test") .assert() .failure() - .stderr(predicate::str::contains( - "The preparation command terminated with a non-zero exit code.", - )); + .stderr(predicate::str::contains("The preparation command failed.")); } #[test] @@ -355,9 +352,40 @@ fn fails_for_unknown_conclude_command() { .arg("echo test") .assert() .failure() - .stderr(predicate::str::contains( - "The conclusion command terminated with a non-zero exit code.", - )); + .stderr(predicate::str::contains("The conclusion command failed.")); +} + +#[cfg(unix)] +#[test] +fn hook_errors_preserve_causes_and_explain_shell_syntax() { + for hook in ["--setup", "--prepare", "--conclude", "--cleanup"] { + for (command, cause) in [ + ( + "some-nonexisting-program-b5d9574198b7e4b12a71fa4747c0a577", + "No such file or directory", + ), + ("printf 'unclosed", "missing closing quote"), + ("false", "terminated with non-zero exit code 1"), + ] { + let assert = hyperfine() + .args(["--runs=1", hook, command, "true"]) + .assert() + .failure() + .stderr(predicate::str::contains(command)) + .stderr(predicate::str::contains(cause)) + .stderr(predicate::str::contains("'-S'/'--shell=default'")) + .stderr(predicate::str::contains("append ' || true'")) + .stderr(predicate::str::contains("--ignore-failure").not()); + if command != "false" { + assert.stderr(predicate::str::contains("non-zero exit code").not()); + } + } + + hyperfine() + .args(["-S", "--runs=1", hook, "false || true", "true"]) + .assert() + .success(); + } } #[cfg(unix)] @@ -368,7 +396,7 @@ fn can_run_failing_commands_with_ignore_failure_option() { .assert() .failure() .stderr(predicate::str::contains( - "Command terminated with non-zero exit code", + "Command 'false' terminated with non-zero exit code", )); hyperfine() @@ -400,7 +428,7 @@ fn can_ignore_specific_exit_codes() { .assert() .failure() .stderr(predicate::str::contains( - "Command terminated with non-zero exit code 2", + "Command 'exit 2' terminated with non-zero exit code 2", )); } @@ -441,7 +469,7 @@ fn can_ignore_multiple_exit_codes() { .assert() .failure() .stderr(predicate::str::contains( - "Command terminated with non-zero exit code 4", + "Command 'exit 4' terminated with non-zero exit code 4", )); }