[nextest-runner] make the runner loop own the test output buffers

Currently, when collecting output from tests, the corresponding futures own the
buffers into which output is being collected. While this works for simpler use
cases, we'd like to be able to dump in-progress output to the terminal in the
future.

To enable this use case, use the "externalized progress" pattern pioneered by
APIs like Tokio's `AsyncReadExt::read_buf`: maintain an external set of buffers
to accumulate output into called `ChildOutputMut`, and write into them within
the corresponding select loops.

In the future, on receiving an instruction to get the current standard
output/standard error, the select statement would be able to look at these
buffers and return whatever's currently in them.

Also do some general cleanup of comments and error reporting.

Cargo.toml

@@ -61,6 +61,7 @@ guppy = "0.17.8"
 home = "0.5.9"
 http = "1.1.0"
 humantime-serde = "1.1.1"
+indenter = "0.3.3"
 indexmap = "2.6.0"
 indicatif = "0.17.8"
 indoc = "2.0.5"

cargo-nextest/src/dispatch.rs

@@ -2145,8 +2145,8 @@ fn extract_description<'a>(stdout: &'a [u8], stderr: &'a [u8]) -> Option<Descrip
             abort_status: None,
             leaked: false,
         },
-        stdout,
-        stderr,
+        Some(stdout),
+        Some(stderr),
     )
 }
 

nextest-runner/Cargo.toml

@@ -35,6 +35,7 @@ guppy.workspace = true
 # added a config.toml there
 home.workspace = true
 humantime-serde.workspace = true
+indenter.workspace = true
 indexmap = { workspace = true, features = ["serde"] }
 indicatif.workspace = true
 is_ci.workspace = true

nextest-runner/src/errors.rs

@@ -18,7 +18,13 @@ use itertools::Itertools;
 use nextest_filtering::errors::FiltersetParseErrors;
 use nextest_metadata::RustBinaryId;
 use smol_str::SmolStr;
-use std::{borrow::Cow, collections::BTreeSet, env::JoinPathsError, fmt, process::ExitStatus};
+use std::{
+    borrow::Cow,
+    collections::BTreeSet,
+    env::JoinPathsError,
+    fmt::{self, Write as _},
+    process::ExitStatus,
+};
 use target_spec_miette::IntoMietteDiagnostic;
 use thiserror::Error;
 
@@ -220,11 +226,11 @@ pub(crate) enum RunTestError {
     #[error("error spawning test process")]
     Spawn(#[source] std::io::Error),
 
-    #[error("error waiting for setup script to exit")]
-    Wait(#[source] std::io::Error),
-
-    #[error("error collecting test output")]
-    CollectOutput(#[from] CollectTestOutputError),
+    #[error("errors while managing test process")]
+    Child {
+        /// The errors that occurred; guaranteed to be non-empty.
+        errors: ErrorList<ChildError>,
+    },
 }
 
 /// An error that occurred while setting up or running a setup script.
@@ -238,13 +244,12 @@ pub(crate) enum SetupScriptError {
     #[error("error executing setup script")]
     ExecFail(#[source] std::io::Error),
 
-    /// An error occurred while collecting the output of the setup script.
-    #[error("error collecting setup script output")]
-    CollectOutput(#[from] CollectTestOutputError),
-
-    /// An error occurred while waiting for the setup script to exit.
-    #[error("error waiting for setup script to exit")]
-    Wait(#[source] std::io::Error),
+    /// One or more errors occurred while managing the child process.
+    #[error("errors while managing setup script process")]
+    Child {
+        /// The errors that occurred; guaranteed to be non-empty.
+        errors: ErrorList<ChildError>,
+    },
 
     /// An error occurred while opening the setup script environment file.
     #[error("error opening environment file `{path}`")]
@@ -277,17 +282,68 @@ pub(crate) enum SetupScriptError {
     EnvFileReservedKey { key: String },
 }
 
-/// An error was returned while collecting test output.
+/// A list of errors that implements `Error`.
+///
+/// In the future, we'll likely want to replace this with a `miette::Diagnostic`-based error, since
+/// that supports multiple causes via "related".
+#[derive(Clone, Debug)]
+pub struct ErrorList<T>(pub Vec<T>);
+
+impl<T: std::error::Error> fmt::Display for ErrorList<T> {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        // If a single error occurred, pretend that this is just that.
+        if self.0.len() == 1 {
+            return write!(f, "{}", self.0[0]);
+        }
+
+        // Otherwise, list all errors.
+        writeln!(f, "{} errors occurred:", self.0.len())?;
+        for error in &self.0 {
+            writeln!(f, "- {}", error)?;
+            // Also display the chain of causes here, since we can't return a single error in the
+            // causes section below.
+            let mut indent = indenter::indented(f).with_str("  ");
+            let mut cause = error.source();
+            while let Some(cause_error) = cause {
+                writeln!(indent, "Caused by: {}", cause_error)?;
+                cause = cause_error.source();
+            }
+        }
+        Ok(())
+    }
+}
+
+impl<T: std::error::Error> std::error::Error for ErrorList<T> {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        if self.0.len() == 1 {
+            self.0[0].source()
+        } else {
+            // More than one error occurred, so we can't return a single error here. Instead, we
+            // return `None` and display the chain of causes in `fmt::Display`.
+            None
+        }
+    }
+}
+
+/// An error was returned during the process of managing a child process.
 #[derive(Debug, Error)]
 #[non_exhaustive]
-pub enum CollectTestOutputError {
+pub enum ChildError {
     /// An error occurred while reading standard output.
     #[error("error reading standard output")]
     ReadStdout(#[source] std::io::Error),
 
     /// An error occurred while reading standard error.
     #[error("error reading standard error")]
     ReadStderr(#[source] std::io::Error),
+
+    /// An error occurred while reading a combined stream.
+    #[error("error reading combined stream")]
+    ReadCombined(#[source] std::io::Error),
+
+    /// An error occurred while waiting for the child process to exit.
+    #[error("error waiting for child process to exit")]
+    Wait(#[source] std::io::Error),
 }
 
 /// An unknown test group was specified in the config.

nextest-runner/src/reporter/aggregator.rs

@@ -303,7 +303,7 @@ fn set_execute_status_props(
     mut out: TestcaseOrRerun<'_>,
 ) {
     match &execute_status.output {
-        Some(TestExecutionOutput::Output(output)) => {
+        TestExecutionOutput::Output(output) => {
             if !is_success {
                 let description = output.heuristic_extract_description(execute_status.result);
                 if let Some(description) = description {
@@ -313,9 +313,13 @@ fn set_execute_status_props(
 
             if store_stdout_stderr {
                 match output {
-                    TestOutput::Split { stdout, stderr } => {
-                        out.set_system_out(stdout.as_str_lossy())
-                            .set_system_err(stderr.as_str_lossy());
+                    TestOutput::Split(split) => {
+                        if let Some(stdout) = &split.stdout {
+                            out.set_system_out(stdout.as_str_lossy());
+                        }
+                        if let Some(stderr) = &split.stderr {
+                            out.set_system_err(stderr.as_str_lossy());
+                        }
                     }
                     TestOutput::Combined { output } => {
                         out.set_system_out(output.as_str_lossy())
@@ -324,15 +328,12 @@ fn set_execute_status_props(
                 }
             }
         }
-        Some(TestExecutionOutput::ExecFail {
+        TestExecutionOutput::ExecFail {
             message,
             description,
-        }) => {
+        } => {
             out.set_message(format!("Test execution failed: {message}"));
             out.set_description(description);
         }
-        None => {
-            out.set_message("Test failed, but output was not captured");
-        }
     }
 }

nextest-runner/src/reporter/displayer.rs

@@ -16,7 +16,7 @@ use crate::{
         AbortStatus, ExecuteStatus, ExecutionDescription, ExecutionResult, ExecutionStatuses,
         FinalRunStats, RetryData, RunStats, RunStatsFailureKind, SetupScriptExecuteStatus,
     },
-    test_output::{TestExecutionOutput, TestOutput, TestSingleOutput},
+    test_output::{ChildSingleOutput, TestExecutionOutput, TestOutput},
 };
 use bstr::ByteSlice;
 use chrono::{DateTime, FixedOffset};
@@ -1319,21 +1319,26 @@ impl<'a> TestReporterImpl<'a> {
             (self.styles.fail, self.styles.fail_output)
         };
 
-        if !run_status.stdout.is_empty() {
-            write!(writer, "\n{}", "--- ".style(header_style))?;
-            write!(writer, "{:21}", "STDOUT:".style(header_style))?;
-            self.write_setup_script(script_id, command, args, writer)?;
-            writeln!(writer, "{}", " ---".style(header_style))?;
+        if let Some(stdout) = &run_status.output.stdout {
+            if !stdout.is_empty() {
+                write!(writer, "\n{}", "--- ".style(header_style))?;
+                write!(writer, "{:21}", "STDOUT:".style(header_style))?;
+                self.write_setup_script(script_id, command, args, writer)?;
+                writeln!(writer, "{}", " ---".style(header_style))?;
 
-            self.write_test_single_output(&run_status.stdout, writer)?;
+                self.write_test_single_output(stdout, writer)?;
+            }
         }
-        if !run_status.stderr.is_empty() {
-            write!(writer, "\n{}", "--- ".style(header_style))?;
-            write!(writer, "{:21}", "STDERR:".style(header_style))?;
-            self.write_setup_script(script_id, command, args, writer)?;
-            writeln!(writer, "{}", " ---".style(header_style))?;
 
-            self.write_test_single_output(&run_status.stderr, writer)?;
+        if let Some(stderr) = &run_status.output.stderr {
+            if !stderr.is_empty() {
+                write!(writer, "\n{}", "--- ".style(header_style))?;
+                write!(writer, "{:21}", "STDERR:".style(header_style))?;
+                self.write_setup_script(script_id, command, args, writer)?;
+                writeln!(writer, "{}", " ---".style(header_style))?;
+
+                self.write_test_single_output(stderr, writer)?;
+            }
         }
 
         writeln!(writer)
@@ -1355,53 +1360,59 @@ impl<'a> TestReporterImpl<'a> {
         };
 
         match &run_status.output {
-            Some(TestExecutionOutput::Output(output)) => {
+            TestExecutionOutput::Output(output) => {
                 let description = if self.styles.is_colorized {
                     output.heuristic_extract_description(run_status.result)
                 } else {
                     None
                 };
 
                 match output {
-                    TestOutput::Split { stdout, stderr } => {
-                        if !stdout.is_empty() {
-                            write!(writer, "\n{}", "--- ".style(header_style))?;
-                            let out_len = self.write_attempt(run_status, header_style, writer)?;
-                            // The width is to align test instances.
-                            write!(
-                                writer,
-                                "{:width$}",
-                                "STDOUT:".style(header_style),
-                                width = (21 - out_len)
-                            )?;
-                            self.write_instance(*test_instance, writer)?;
-                            writeln!(writer, "{}", " ---".style(header_style))?;
+                    TestOutput::Split(split) => {
+                        if let Some(stdout) = &split.stdout {
+                            if !stdout.is_empty() {
+                                write!(writer, "\n{}", "--- ".style(header_style))?;
+                                let out_len =
+                                    self.write_attempt(run_status, header_style, writer)?;
+                                // The width is to align test instances.
+                                write!(
+                                    writer,
+                                    "{:width$}",
+                                    "STDOUT:".style(header_style),
+                                    width = (21 - out_len)
+                                )?;
+                                self.write_instance(*test_instance, writer)?;
+                                writeln!(writer, "{}", " ---".style(header_style))?;
 
-                            self.write_test_single_output_with_description(
-                                stdout,
-                                description.and_then(|d| d.stdout_subslice()),
-                                writer,
-                            )?;
+                                self.write_test_single_output_with_description(
+                                    stdout,
+                                    description.and_then(|d| d.stdout_subslice()),
+                                    writer,
+                                )?;
+                            }
                         }
 
-                        if !stderr.is_empty() {
-                            write!(writer, "\n{}", "--- ".style(header_style))?;
-                            let out_len = self.write_attempt(run_status, header_style, writer)?;
-                            // The width is to align test instances.
-                            write!(
-                                writer,
-                                "{:width$}",
-                                "STDERR:".style(header_style),
-                                width = (21 - out_len)
-                            )?;
-                            self.write_instance(*test_instance, writer)?;
-                            writeln!(writer, "{}", " ---".style(header_style))?;
+                        if let Some(stderr) = &split.stderr {
+                            if !stderr.is_empty() {
+                                write!(writer, "\n{}", "--- ".style(header_style))?;
+                                let out_len =
+                                    self.write_attempt(run_status, header_style, writer)?;
+                                // The width is to align test instances.
+                                write!(
+                                    writer,
+                                    "{:width$}",
+                                    "STDERR:".style(header_style),
+                                    width = (21 - out_len)
+                                )?;
+                                self.write_instance(*test_instance, writer)?;
+                                writeln!(writer, "{}", " ---".style(header_style))?;
 
-                            self.write_test_single_output_with_description(
-                                stderr,
-                                description.and_then(|d| d.stderr_subslice()),
-                                writer,
-                            )?;
+                                self.write_test_single_output_with_description(
+                                    stderr,
+                                    description.and_then(|d| d.stderr_subslice()),
+                                    writer,
+                                )?;
+                            }
                         }
                     }
                     TestOutput::Combined { output } => {
@@ -1428,7 +1439,7 @@ impl<'a> TestReporterImpl<'a> {
                 }
             }
 
-            Some(TestExecutionOutput::ExecFail { description, .. }) => {
+            TestExecutionOutput::ExecFail { description, .. } => {
                 write!(writer, "\n{}", "--- ".style(header_style))?;
                 let out_len = self.write_attempt(run_status, header_style, writer)?;
                 // The width is to align test instances.
@@ -1443,10 +1454,6 @@ impl<'a> TestReporterImpl<'a> {
 
                 writeln!(writer, "{description}")?;
             }
-
-            None => {
-                // The output wasn't captured.
-            }
         }
 
         writeln!(writer)
@@ -1455,7 +1462,7 @@ impl<'a> TestReporterImpl<'a> {
     /// Writes a test output to the writer.
     fn write_test_single_output(
         &self,
-        output: &TestSingleOutput,
+        output: &ChildSingleOutput,
         writer: &mut dyn Write,
     ) -> io::Result<()> {
         // SAFETY: The description is not provided.
@@ -1468,7 +1475,7 @@ impl<'a> TestReporterImpl<'a> {
     /// The description must be a subslice of the output.
     fn write_test_single_output_with_description(
         &self,
-        output: &TestSingleOutput,
+        output: &ChildSingleOutput,
         description: Option<ByteSubslice<'_>>,
         writer: &mut dyn Write,
     ) -> io::Result<()> {