From 5e77f2b69a70941f7447dc7a19369c9960bfbf2a Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 21:50:32 +0800 Subject: [PATCH 1/5] Add test_ignore_and_pipefail() test suite over most entry points --- tests/test_macros.rs | 106 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 4c87cf0..11a9341 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -160,6 +160,112 @@ fn test_pipe() { set_pipefail(true); } +#[test] +fn test_ignore_and_pipefail() { + struct TestCase { + /// Run the test case, returning whether the result `.is_ok()`. + code: fn() -> bool, + /// Stringified version of `code`, for identifying assertion failures. + code_str: &'static str, + /// Do we expect `.is_ok()` when pipefail is on? + expected_ok_pipefail_on: bool, + /// Do we expect `.is_ok()` when pipefail is off? + expected_ok_pipefail_off: bool, + } + /// Make a function for [TestCase::code]. + /// + /// Usage: `code!((macro!(command)).extra)` + /// - `(macro!(command)).extra` is an expression of type CmdResult + macro_rules! code { + (($macro:tt $bang:tt ($($command:tt)+)) $($after:tt)*) => { + || $macro$bang($($command)+)$($after)*.is_ok() + }; + } + /// Make a string for [TestCase::code_str]. + /// + /// Usage: `code_str!((macro!(command)).extra)` + /// - `(macro!(command)).extra` is an expression of type CmdResult + macro_rules! code_str { + (($macro:tt $bang:tt ($($command:tt)+)) $($after:tt)*) => { + stringify!($macro$bang($($command)+)$($after)*.is_ok()) + }; + } + /// Make a [TestCase]. + /// Usage: `test_case!(true/false, true/false, (macro!(command)).extra)` + /// - the first `true/false` is TestCase::expected_ok_pipefail_on + /// - the second `true/false` is TestCase::expected_ok_pipefail_off + /// - `(macro!(command)).extra` is an expression of type CmdResult + macro_rules! test_case { + ($expected_ok_pipefail_on:expr, $expected_ok_pipefail_off:expr, ($macro:tt $bang:tt ($($command:tt)+)) $($after:tt)*) => { + TestCase { + code: code!(($macro $bang ($($command)+)) $($after)*), + code_str: code_str!(($macro $bang ($($command)+)) $($after)*), + expected_ok_pipefail_on: $expected_ok_pipefail_on, + expected_ok_pipefail_off: $expected_ok_pipefail_off, + } + }; + } + /// Generate test cases for the given entry point. + /// For each test case, every entry point should yield the same results. + macro_rules! test_cases_for_entry_point { + (($macro:tt $bang:tt (...)) $($after:tt)*) => { + &[ + // Use result of last command in pipeline, if all others exit successfully. + test_case!(true, true, ($macro $bang (true)) $($after)*), + test_case!(false, false, ($macro $bang (false)) $($after)*), + test_case!(true, true, ($macro $bang (true | true)) $($after)*), + test_case!(false, false, ($macro $bang (true | false)) $($after)*), + // Use failure of other commands, if pipefail is on. + test_case!(false, true, ($macro $bang (false | true)) $($after)*), + // Use failure of last command in pipeline. + test_case!(false, false, ($macro $bang (false | false)) $($after)*), + // Ignore all failures, when using `ignore` command. + test_case!(true, true, ($macro $bang (ignore true)) $($after)*), + test_case!(true, true, ($macro $bang (ignore false)) $($after)*), + test_case!(true, true, ($macro $bang (ignore true | true)) $($after)*), + test_case!(true, true, ($macro $bang (ignore true | false)) $($after)*), + test_case!(true, true, ($macro $bang (ignore false | true)) $($after)*), + test_case!(true, true, ($macro $bang (ignore false | false)) $($after)*), + ] + }; + } + + let test_cases: &[&[TestCase]] = &[ + test_cases_for_entry_point!((run_cmd!(...))), + test_cases_for_entry_point!((run_fun!(...)).map(|_stdout| ())), + test_cases_for_entry_point!((spawn!(...)).unwrap().wait()), + test_cases_for_entry_point!((spawn_with_output!(...)).unwrap().wait_with_all().0), + test_cases_for_entry_point!((spawn_with_output!(...)) + .unwrap() + .wait_with_output() + .map(|_stdout| ())), + test_cases_for_entry_point!((spawn_with_output!(...)) + .unwrap() + .wait_with_raw_output(&mut vec![])), + // FIXME: wait_with_pipe() is currently busted + // test_cases_for_entry_point!((spawn_with_output!(...)) + // .unwrap() + // .wait_with_pipe(&mut |_stdout| {})), + ]; + + for case in test_cases.iter().flat_map(|items| items.iter()) { + assert_eq!( + (case.code)(), + case.expected_ok_pipefail_on, + "{} when pipefail is on", + case.code_str + ); + set_pipefail(false); + assert_eq!( + (case.code)(), + case.expected_ok_pipefail_off, + "{} when pipefail is off", + case.code_str + ); + set_pipefail(true); + } +} + #[test] /// ```compile_fail /// run_cmd!(ls > >&1).unwrap(); From 9aa8698b51147bb14b579e07fddc446efa1b5b00 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 22:12:37 +0800 Subject: [PATCH 2/5] Print all assertions that failed in test_ignore_and_pipefail() --- tests/test_macros.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/test_macros.rs b/tests/test_macros.rs index 11a9341..ec00e76 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -248,15 +248,29 @@ fn test_ignore_and_pipefail() { // .wait_with_pipe(&mut |_stdout| {})), ]; + macro_rules! check_eq { + ($left:expr, $right:expr, $($rest:tt)+) => {{ + let left = $left; + let right = $right; + if left != right { + eprintln!("assertion failed ({} != {}): {}", left, right, format!($($rest)+)); + false + } else { + true + } + }}; + } + + let mut ok = true; for case in test_cases.iter().flat_map(|items| items.iter()) { - assert_eq!( + ok &= check_eq!( (case.code)(), case.expected_ok_pipefail_on, "{} when pipefail is on", case.code_str ); set_pipefail(false); - assert_eq!( + ok &= check_eq!( (case.code)(), case.expected_ok_pipefail_off, "{} when pipefail is off", @@ -264,6 +278,8 @@ fn test_ignore_and_pipefail() { ); set_pipefail(true); } + + assert!(ok); } #[test] From 61e0a8091dd87577eb962a1c09337f49ea22f4c8 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 22:34:51 +0800 Subject: [PATCH 3/5] Remove old pipefail tests, which are covered by new test suite --- tests/test_macros.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tests/test_macros.rs b/tests/test_macros.rs index ec00e76..c70f0a3 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -150,14 +150,6 @@ fn test_pipe() { let wc_cmd = "wc"; assert!(run_cmd!(ls | $wc_cmd).is_ok()); - - // test pipefail - assert!(run_cmd!(false | true).is_err()); - assert!(run_fun!(false | true).is_err()); - assert!(run_fun!(ignore false | true).is_ok()); - set_pipefail(false); - assert!(run_fun!(false | true).is_ok()); - set_pipefail(true); } #[test] From 2c7fc8cd71cbdab190928e995e5ad30ea5e32d00 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 22:35:44 +0800 Subject: [PATCH 4/5] Work around bug where set_pipefail() is not thread safe --- tests/test_macros.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_macros.rs b/tests/test_macros.rs index c70f0a3..ff4dbce 100644 --- a/tests/test_macros.rs +++ b/tests/test_macros.rs @@ -150,10 +150,9 @@ fn test_pipe() { let wc_cmd = "wc"; assert!(run_cmd!(ls | $wc_cmd).is_ok()); -} -#[test] -fn test_ignore_and_pipefail() { + // test `ignore` command and pipefail mode + // FIXME: make set_pipefail() thread safe, then move this to a separate test_ignore_and_pipefail() struct TestCase { /// Run the test case, returning whether the result `.is_ok()`. code: fn() -> bool, From 7a8dbd8b7cad412d60c0891a91560480f667d7d0 Mon Sep 17 00:00:00 2001 From: Delan Azabani Date: Sat, 2 Aug 2025 22:14:17 +0800 Subject: [PATCH 5/5] Fix handling of `ignore` in wait() and wait_with_all() --- src/child.rs | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/child.rs b/src/child.rs index 6e4f73d..f9d4d1c 100644 --- a/src/child.rs +++ b/src/child.rs @@ -31,13 +31,13 @@ impl CmdChildren { /// Waits for the children processes to exit completely, returning the status that they exited with. pub fn wait(&mut self) -> CmdResult { - // wait for the last child result - let handle = self.children.pop().unwrap(); - if let Err(e) = handle.wait(true) { - let _ = Self::wait_children(&mut self.children); - return Err(e); - } - Self::wait_children(&mut self.children) + let last_child = self.children.pop().unwrap(); + let last_child_res = last_child.wait(true); + let other_children_res = Self::wait_children(&mut self.children); + + self.ignore_error + .then_some(Ok(())) + .unwrap_or(last_child_res.and(other_children_res)) } fn wait_children(children: &mut Vec) -> CmdResult { @@ -149,21 +149,23 @@ impl FunChildren { } fn inner_wait_with_all(&mut self, capture_stderr: bool) -> (CmdResult, String, String) { - // wait for the last child result - let last_handle = self.children.pop().unwrap(); - let mut stdout_buf = Vec::new(); + let mut stdout = Vec::new(); let mut stderr = String::new(); - let last_res = last_handle.wait_with_all(capture_stderr, &mut stdout_buf, &mut stderr); - let res = CmdChildren::wait_children(&mut self.children); - let mut stdout: String = String::from_utf8_lossy(&stdout_buf).into(); + + let last_child = self.children.pop().unwrap(); + let last_child_res = last_child.wait_with_all(capture_stderr, &mut stdout, &mut stderr); + let other_children_res = CmdChildren::wait_children(&mut self.children); + let cmd_result = self + .ignore_error + .then_some(Ok(())) + .unwrap_or(last_child_res.and(other_children_res)); + + let mut stdout: String = String::from_utf8_lossy(&stdout).into(); if stdout.ends_with('\n') { stdout.pop(); } - if res.is_err() && !self.ignore_error && process::pipefail_enabled() { - (res, stdout, stderr) - } else { - (last_res, stdout, stderr) - } + + (cmd_result, stdout, stderr) } }