From 73f5b2fe508bd9fc46dc9a0432e582058047da9a Mon Sep 17 00:00:00 2001 From: alleyshairu Date: Fri, 22 Nov 2024 02:20:32 +1100 Subject: [PATCH 1/3] Improve error message when `run_command_async` fails to find a command --- test-helpers/src/lib.rs | 59 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/test-helpers/src/lib.rs b/test-helpers/src/lib.rs index 5cf831393..117d547ff 100644 --- a/test-helpers/src/lib.rs +++ b/test-helpers/src/lib.rs @@ -38,6 +38,26 @@ pub fn run_command(command: &str, args: &[&str]) -> Result { } } +fn command_not_found_error_message(command: &str, args: &[&str]) -> String { + // Maps a command to its associated dependency; however, if the name of the command and dependency is the same, we just use the command name. + // Currently, the only use case is mapping `npm` to its dependency `nodejs`. + let dependency = match command { + "npm" => "nodejs", + _ => command, + }; + + let args_part = if !args.is_empty() { + format!(" {}", args.join(" ")) + } else { + String::new() + }; + + format!( + "Attempted to run the command `{}{}` but {} does not exist. Have you installed {}?", + command, args_part, command, dependency + ) +} + pub async fn run_command_async(current_dir: &Path, command: &str, args: &[&str]) { let output = tokio::process::Command::new(command) .args(args) @@ -45,9 +65,48 @@ pub async fn run_command_async(current_dir: &Path, command: &str, args: &[&str]) .kill_on_drop(true) .status() .await + .map_err(|e| { + if e.kind() == std::io::ErrorKind::NotFound { + return std::io::Error::new( + std::io::ErrorKind::NotFound, + command_not_found_error_message(command, args), + ); + } + e + }) .unwrap(); if !output.success() { panic!("command {command} {args:?} failed. See above output.") } } + +#[cfg(test)] +mod tests { + use super::*; + use futures_util::FutureExt; + + #[tokio::test] + async fn test_run_command_async_not_found() { + let dir = Path::new(env!("CARGO_MANIFEST_DIR")); + let command = "shotover_non_existent_command"; + let args = &["arg1", "arg2"]; + let result = std::panic::AssertUnwindSafe(run_command_async(dir, command, args)) + .catch_unwind() + .await; + + assert!(result.is_err(), "Expected a panic but none occurred"); + + if let Err(panic_info) = result { + if let Some(error_message) = panic_info.downcast_ref::() { + assert!( + error_message.contains(&command_not_found_error_message(command, args)), + "Error message did not contain the expected NotFound error: {}", + error_message + ); + } else { + panic!("Panic payload was not a string: {:?}", panic_info); + } + } + } +} From f0a9bd7b57f0390d993862dee483800e52452d3d Mon Sep 17 00:00:00 2001 From: alleyshairu Date: Fri, 22 Nov 2024 09:54:14 +1100 Subject: [PATCH 2/3] transform error into anyhow error and fix test by comparing against desired string --- test-helpers/src/lib.rs | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/test-helpers/src/lib.rs b/test-helpers/src/lib.rs index 117d547ff..18da8e16f 100644 --- a/test-helpers/src/lib.rs +++ b/test-helpers/src/lib.rs @@ -6,7 +6,7 @@ pub mod mock_cassandra; pub mod shotover_process; mod test_tracing; -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, Error, Result}; use std::path::Path; use subprocess::{Exec, Redirection}; @@ -67,12 +67,10 @@ pub async fn run_command_async(current_dir: &Path, command: &str, args: &[&str]) .await .map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { - return std::io::Error::new( - std::io::ErrorKind::NotFound, - command_not_found_error_message(command, args), - ); + return Error::msg(command_not_found_error_message(command, args)); } - e + + Error::from(e) }) .unwrap(); @@ -87,7 +85,7 @@ mod tests { use futures_util::FutureExt; #[tokio::test] - async fn test_run_command_async_not_found() { + async fn test_run_command_async_not_found_message() { let dir = Path::new(env!("CARGO_MANIFEST_DIR")); let command = "shotover_non_existent_command"; let args = &["arg1", "arg2"]; @@ -100,8 +98,8 @@ mod tests { if let Err(panic_info) = result { if let Some(error_message) = panic_info.downcast_ref::() { assert!( - error_message.contains(&command_not_found_error_message(command, args)), - "Error message did not contain the expected NotFound error: {}", + error_message.contains("Attempted to run the command `shotover_non_existent_command arg1 arg2` but shotover_non_existent_command does not exist. Have you installed shotover_non_existent_command?"), + "Error message did not contain the expected NotFound error got: {}", error_message ); } else { From e569cbe88cf0fc989b95312cb664eefedbe61c11 Mon Sep 17 00:00:00 2001 From: alleyshairu Date: Fri, 22 Nov 2024 11:34:15 +1100 Subject: [PATCH 3/3] use anyhow! macro for error creation --- test-helpers/src/lib.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/test-helpers/src/lib.rs b/test-helpers/src/lib.rs index 18da8e16f..8fb6b1e3d 100644 --- a/test-helpers/src/lib.rs +++ b/test-helpers/src/lib.rs @@ -38,7 +38,7 @@ pub fn run_command(command: &str, args: &[&str]) -> Result { } } -fn command_not_found_error_message(command: &str, args: &[&str]) -> String { +fn command_not_found_error(command: &str, args: &[&str]) -> Error { // Maps a command to its associated dependency; however, if the name of the command and dependency is the same, we just use the command name. // Currently, the only use case is mapping `npm` to its dependency `nodejs`. let dependency = match command { @@ -52,9 +52,12 @@ fn command_not_found_error_message(command: &str, args: &[&str]) -> String { String::new() }; - format!( + anyhow!( "Attempted to run the command `{}{}` but {} does not exist. Have you installed {}?", - command, args_part, command, dependency + command, + args_part, + command, + dependency ) } @@ -67,10 +70,10 @@ pub async fn run_command_async(current_dir: &Path, command: &str, args: &[&str]) .await .map_err(|e| { if e.kind() == std::io::ErrorKind::NotFound { - return Error::msg(command_not_found_error_message(command, args)); + return command_not_found_error(command, args); } - Error::from(e) + anyhow!(e) }) .unwrap();