chore(tests): ability to capture stdout and stderr separately (#18035)

This is to allow making assertions on stdout and stderr separately.

cli/tests/integration/bench_tests.rs

@@ -196,7 +196,7 @@ fn recursive_permissions_pledge() {
     .run();
   output.assert_exit_code(1);
   assert_contains!(
-    output.text(),
+    output.combined_output(),
     "pledge test permissions called before restoring previous pledge"
   );
 }

runtime/ops/process.rs

@@ -1,7 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
 use super::check_unstable;
-use super::signal;
 use crate::permissions::PermissionsContainer;
 use deno_core::error::AnyError;
 use deno_core::op;
@@ -565,7 +564,7 @@ mod deprecated {
 
   #[cfg(unix)]
   pub fn kill(pid: i32, signal: &str) -> Result<(), AnyError> {
-    let signo = super::signal::signal_str_to_int(signal)?;
+    let signo = super::super::signal::signal_str_to_int(signal)?;
     use nix::sys::signal::kill as unix_kill;
     use nix::sys::signal::Signal;
     use nix::unistd::Pid;
@@ -593,8 +592,8 @@ mod deprecated {
     } else if pid <= 0 {
       Err(type_error("Invalid pid"))
     } else {
-      // SAFETY: winapi call
       let handle =
+        // SAFETY: winapi call
         unsafe { OpenProcess(PROCESS_TERMINATE, FALSE, pid as DWORD) };
 
       if handle.is_null() {

test_util/src/builders.rs

@@ -168,6 +168,7 @@ impl TestContext {
       envs: Default::default(),
       env_clear: Default::default(),
       cwd: Default::default(),
+      split_output: false,
       context: self.clone(),
     }
   }
@@ -181,6 +182,7 @@ pub struct TestCommandBuilder {
   envs: HashMap<String, String>,
   env_clear: bool,
   cwd: Option<String>,
+  split_output: bool,
   context: TestContext,
 }
 
@@ -205,6 +207,15 @@ impl TestCommandBuilder {
     self
   }
 
+  /// Splits the output into stdout and stderr rather than having them combined.
+  pub fn split_output(&mut self) -> &mut Self {
+    // Note: it was previously attempted to capture stdout & stderr separately
+    // then forward the output to a combined pipe, but this was found to be
+    // too racy compared to providing the same combined pipe to both.
+    self.split_output = true;
+    self
+  }
+
   pub fn env(
     &mut self,
     key: impl AsRef<str>,
@@ -227,6 +238,23 @@ impl TestCommandBuilder {
   }
 
   pub fn run(&self) -> TestCommandOutput {
+    fn read_pipe_to_string(mut pipe: os_pipe::PipeReader) -> String {
+      let mut output = String::new();
+      pipe.read_to_string(&mut output).unwrap();
+      output
+    }
+
+    fn sanitize_output(text: String, args: &[String]) -> String {
+      let mut text = strip_ansi_codes(&text).to_string();
+      // deno test's output capturing flushes with a zero-width space in order to
+      // synchronize the output pipes. Occassionally this zero width space
+      // might end up in the output so strip it from the output comparison here.
+      if args.first().map(|s| s.as_str()) == Some("test") {
+        text = text.replace('\u{200B}', "");
+      }
+      text
+    }
+
     let cwd = self.cwd.as_ref().or(self.context.cwd.as_ref());
     let cwd = if self.context.use_temp_cwd {
       assert!(cwd.is_none());
@@ -256,7 +284,6 @@ impl TestCommandBuilder {
       arg.replace("$TESTDATA", &self.context.testdata_dir.to_string_lossy())
     })
     .collect::<Vec<_>>();
-    let (mut reader, writer) = pipe().unwrap();
     let command_name = self
       .command_name
       .as_ref()
@@ -284,9 +311,25 @@ impl TestCommandBuilder {
     });
     command.current_dir(cwd);
     command.stdin(Stdio::piped());
-    let writer_clone = writer.try_clone().unwrap();
+
-    command.stderr(writer_clone);
+    let (combined_reader, std_out_err_handle) = if self.split_output {
-    command.stdout(writer);
+      let (stdout_reader, stdout_writer) = pipe().unwrap();
+      let (stderr_reader, stderr_writer) = pipe().unwrap();
+      command.stdout(stdout_writer);
+      command.stderr(stderr_writer);
+      (
+        None,
+        Some((
+          std::thread::spawn(move || read_pipe_to_string(stdout_reader)),
+          std::thread::spawn(move || read_pipe_to_string(stderr_reader)),
+        )),
+      )
+    } else {
+      let (combined_reader, combined_writer) = pipe().unwrap();
+      command.stdout(combined_writer.try_clone().unwrap());
+      command.stderr(combined_writer);
+      (Some(combined_reader), None)
+    };
 
     let mut process = command.spawn().unwrap();
 
@@ -301,10 +344,16 @@ impl TestCommandBuilder {
     // and dropping it closes them.
     drop(command);
 
-    let mut actual = String::new();
+    let combined = combined_reader
-    reader.read_to_string(&mut actual).unwrap();
+      .map(|pipe| sanitize_output(read_pipe_to_string(pipe), &args));
 
-    let status = process.wait().expect("failed to finish process");
+    let status = process.wait().unwrap();
+    let std_out_err = std_out_err_handle.map(|(stdout, stderr)| {
+      (
+        sanitize_output(stdout.join().unwrap(), &args),
+        sanitize_output(stderr.join().unwrap(), &args),
+      )
+    });
     let exit_code = status.code();
     #[cfg(unix)]
     let signal = {
@@ -314,33 +363,30 @@ impl TestCommandBuilder {
     #[cfg(not(unix))]
     let signal = None;
 
-    actual = strip_ansi_codes(&actual).to_string();
-
-    // deno test's output capturing flushes with a zero-width space in order to
-    // synchronize the output pipes. Occassionally this zero width space
-    // might end up in the output so strip it from the output comparison here.
-    if args.first().map(|s| s.as_str()) == Some("test") {
-      actual = actual.replace('\u{200B}', "");
-    }
-
     TestCommandOutput {
       exit_code,
       signal,
-      text: actual,
+      combined,
+      std_out_err,
       testdata_dir: self.context.testdata_dir.clone(),
       asserted_exit_code: RefCell::new(false),
-      asserted_text: RefCell::new(false),
+      asserted_stdout: RefCell::new(false),
+      asserted_stderr: RefCell::new(false),
+      asserted_combined: RefCell::new(false),
       _test_context: self.context.clone(),
     }
   }
 }
 
 pub struct TestCommandOutput {
-  text: String,
+  combined: Option<String>,
+  std_out_err: Option<(String, String)>,
   exit_code: Option<i32>,
   signal: Option<i32>,
   testdata_dir: PathBuf,
-  asserted_text: RefCell<bool>,
+  asserted_stdout: RefCell<bool>,
+  asserted_stderr: RefCell<bool>,
+  asserted_combined: RefCell<bool>,
   asserted_exit_code: RefCell<bool>,
   // keep alive for the duration of the output reference
   _test_context: TestContext,
@@ -348,6 +394,17 @@ pub struct TestCommandOutput {
 
 impl Drop for TestCommandOutput {
   fn drop(&mut self) {
+    fn panic_unasserted_output(text: &str) {
+      println!("OUTPUT\n{text}\nOUTPUT");
+      panic!(
+        concat!(
+          "The non-empty text of the command was not asserted at {}. ",
+          "Call `output.skip_output_check()` to skip if necessary.",
+        ),
+        failed_position()
+      );
+    }
+
     if std::thread::panicking() {
       return;
     }
@@ -359,15 +416,20 @@ impl Drop for TestCommandOutput {
         failed_position(),
       )
     }
-    if !*self.asserted_text.borrow() && !self.text.is_empty() {
+
-      println!("OUTPUT\n{}\nOUTPUT", self.text);
+    // either the combined output needs to be asserted or both stdout and stderr
-      panic!(
+    if let Some(combined) = &self.combined {
-        concat!(
+      if !*self.asserted_combined.borrow() && !combined.is_empty() {
-          "The non-empty text of the command was not asserted. ",
+        panic_unasserted_output(combined);
-          "Call `output.skip_output_check()` to skip if necessary at {}.",
+      }
-        ),
+    }
-        failed_position()
+    if let Some((stdout, stderr)) = &self.std_out_err {
-      );
+      if !*self.asserted_stdout.borrow() && !stdout.is_empty() {
+        panic_unasserted_output(stdout);
+      }
+      if !*self.asserted_stderr.borrow() && !stderr.is_empty() {
+        panic_unasserted_output(stderr);
+      }
     }
   }
 }
@@ -378,7 +440,9 @@ impl TestCommandOutput {
   }
 
   pub fn skip_output_check(&self) {
-    *self.asserted_text.borrow_mut() = true;
+    *self.asserted_combined.borrow_mut() = true;
+    *self.asserted_stdout.borrow_mut() = true;
+    *self.asserted_stderr.borrow_mut() = true;
   }
 
   pub fn skip_exit_code_check(&self) {
@@ -394,9 +458,30 @@ impl TestCommandOutput {
     self.signal
   }
 
-  pub fn text(&self) -> &str {
+  pub fn combined_output(&self) -> &str {
     self.skip_output_check();
-    &self.text
+    self
+      .combined
+      .as_deref()
+      .expect("not available since .split_output() was called")
+  }
+
+  pub fn stdout(&self) -> &str {
+    *self.asserted_stdout.borrow_mut() = true;
+    self
+      .std_out_err
+      .as_ref()
+      .map(|(stdout, _)| stdout.as_str())
+      .expect("call .split_output() on the builder")
+  }
+
+  pub fn stderr(&self) -> &str {
+    *self.asserted_stderr.borrow_mut() = true;
+    self
+      .std_out_err
+      .as_ref()
+      .map(|(_, stderr)| stderr.as_str())
+      .expect("call .split_output() on the builder")
   }
 
   pub fn assert_exit_code(&self, expected_exit_code: i32) -> &Self {
@@ -404,7 +489,7 @@ impl TestCommandOutput {
 
     if let Some(exit_code) = &actual_exit_code {
       if *exit_code != expected_exit_code {
-        println!("OUTPUT\n{}\nOUTPUT", self.text());
+        self.print_output();
         panic!(
           "bad exit code, expected: {:?}, actual: {:?} at {}",
           expected_exit_code,
@@ -413,7 +498,7 @@ impl TestCommandOutput {
         );
       }
     } else {
-      println!("OUTPUT\n{}\nOUTPUT", self.text());
+      self.print_output();
       if let Some(signal) = self.signal() {
         panic!(
           "process terminated by signal, expected exit code: {:?}, actual signal: {:?} at {}",
@@ -433,29 +518,79 @@ impl TestCommandOutput {
     self
   }
 
-  pub fn assert_matches_text(&self, expected_text: impl AsRef<str>) -> &Self {
+  pub fn print_output(&self) {
-    let expected_text = expected_text.as_ref();
+    if let Some(combined) = &self.combined {
-    let actual = self.text();
+      println!("OUTPUT\n{combined}\nOUTPUT");
-
+    } else if let Some((stdout, stderr)) = &self.std_out_err {
-    if !expected_text.contains("[WILDCARD]") {
+      println!("STDOUT OUTPUT\n{stdout}\nSTDOUT OUTPUT");
-      assert_eq!(actual, expected_text, "at {}", failed_position());
+      println!("STDERR OUTPUT\n{stderr}\nSTDERR OUTPUT");
-    } else if !wildcard_match(expected_text, actual) {
-      println!("OUTPUT START\n{actual}\nOUTPUT END");
-      println!("EXPECTED START\n{expected_text}\nEXPECTED END");
-      panic!("pattern match failed at {}", failed_position());
     }
+  }
 
-    self
+  pub fn assert_matches_text(&self, expected_text: impl AsRef<str>) -> &Self {
+    self.inner_assert_matches_text(self.combined_output(), expected_text)
   }
 
   pub fn assert_matches_file(&self, file_path: impl AsRef<Path>) -> &Self {
+    self.inner_assert_matches_file(self.combined_output(), file_path)
+  }
+
+  pub fn assert_stdout_matches_text(
+    &self,
+    expected_text: impl AsRef<str>,
+  ) -> &Self {
+    self.inner_assert_matches_text(self.stdout(), expected_text)
+  }
+
+  pub fn assert_stdout_matches_file(
+    &self,
+    file_path: impl AsRef<Path>,
+  ) -> &Self {
+    self.inner_assert_matches_file(self.stdout(), file_path)
+  }
+
+  pub fn assert_stderr_matches_text(
+    &self,
+    expected_text: impl AsRef<str>,
+  ) -> &Self {
+    self.inner_assert_matches_text(self.stderr(), expected_text)
+  }
+
+  pub fn assert_stderrr_matches_file(
+    &self,
+    file_path: impl AsRef<Path>,
+  ) -> &Self {
+    self.inner_assert_matches_file(self.stderr(), file_path)
+  }
+
+  fn inner_assert_matches_text(
+    &self,
+    actual: &str,
+    expected: impl AsRef<str>,
+  ) -> &Self {
+    let expected = expected.as_ref();
+    if !expected.contains("[WILDCARD]") {
+      assert_eq!(actual, expected, "at {}", failed_position());
+    } else if !wildcard_match(expected, actual) {
+      println!("OUTPUT START\n{actual}\nOUTPUT END");
+      println!("EXPECTED START\n{expected}\nEXPECTED END");
+      panic!("pattern match failed at {}", failed_position());
+    }
+    self
+  }
+
+  fn inner_assert_matches_file(
+    &self,
+    actual: &str,
+    file_path: impl AsRef<Path>,
+  ) -> &Self {
     let output_path = self.testdata_dir().join(file_path);
     println!("output path {}", output_path.display());
     let expected_text =
       std::fs::read_to_string(&output_path).unwrap_or_else(|err| {
         panic!("failed loading {}\n\n{err:#}", output_path.display())
       });
-    self.assert_matches_text(expected_text)
+    self.inner_assert_matches_text(actual, expected_text)
   }
 }