Skip to content

Fix handling of ignore in wait() and wait_with_all() #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Aug 3, 2025
Merged

Fix handling of ignore in wait() and wait_with_all() #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5e77f2b
Add test_ignore_and_pipefail() test suite over most entry points
delan Aug 2, 2025
9aa8698
Print all assertions that failed in test_ignore_and_pipefail()
delan Aug 2, 2025
61e0a80
Remove old pipefail tests, which are covered by new test suite
delan Aug 2, 2025
2c7fc8c
Work around bug where set_pipefail() is not thread safe
delan Aug 2, 2025
7a8dbd8
Fix handling of `ignore` in wait() and wait_with_all()
delan Aug 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 20 additions & 18 deletions src/child.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is precise and thanks for the clean up.

.unwrap_or(last_child_res.and(other_children_res))
}

fn wait_children(children: &mut Vec<CmdChild>) -> CmdResult {
Expand Down Expand Up @@ -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)
}
}

Expand Down
127 changes: 120 additions & 7 deletions tests/test_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,13 +151,126 @@ 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 `ignore` command and pipefail mode
// FIXME: make set_pipefail() thread safe, then move this to a separate test_ignore_and_pipefail()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you thought about using sealed_test to make the tests work ? Or we can make the CI to run tests sequentially

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could do that, but i think it’s possible to fix the thread unsafety, and doing so would make the API more reliable. i’ll do that in a separate patch.

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

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()) {
ok &= check_eq!(
(case.code)(),
case.expected_ok_pipefail_on,
"{} when pipefail is on",
case.code_str
);
set_pipefail(false);
ok &= check_eq!(
(case.code)(),
case.expected_ok_pipefail_off,
"{} when pipefail is off",
case.code_str
);
set_pipefail(true);
}

assert!(ok);
}

#[test]
Expand Down