Improve command and hook failure diagnostics

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<CmdFailureAction>,
+    options: &Options,
     command_output_policy: &CommandOutputPolicy,
     command_name: &str,
     collect_hardware_counters: bool,
 ) -> Result<Measurement> {
-    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,

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<Measurement> {
@@ -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<Measurement> {
-        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<Measurement> {
-        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<()> {

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",
         ));
 }