Merge pull request #82 from delan/fix-ignore

Fix handling of `ignore` in wait() and wait_with_all()

Author: tao-guo

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<CmdChild>) -> 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)
     }
 }
 

tests/test_macros.rs

@@ -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()
+    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]