[nextest-runner] improve data model for child error management

I realized that we need a central way to identify and manage all the different
ways in which a test or script can error out. Add a `UnitErrorDescription`
struct which is responsible for all this management.

cargo-nextest/src/dispatch.rs

@@ -29,14 +29,11 @@ use nextest_runner::{
     platform::{BuildPlatforms, HostPlatform, PlatformLibdir, TargetPlatform},
     redact::Redactor,
     reporter::{
-        heuristic_extract_description, highlight_end, structured, DescriptionKind,
-        FinalStatusLevel, StatusLevel, TestOutputDisplay, TestReporterBuilder,
+        heuristic_slice_from_output, highlight_end, structured, FinalStatusLevel, StatusLevel,
+        TestOutputDisplay, TestOutputErrorSlice, TestReporterBuilder,
     },
     reuse_build::{archive_to_file, ArchiveReporter, PathMapper, ReuseBuildInfo},
-    runner::{
-        configure_handle_inheritance, ExecutionResult, FinalRunStats, RunStatsFailureKind,
-        TestRunnerBuilder,
-    },
+    runner::{configure_handle_inheritance, FinalRunStats, RunStatsFailureKind, TestRunnerBuilder},
     show_config::{ShowNextestVersion, ShowTestGroupSettings, ShowTestGroups, ShowTestGroupsMode},
     signal::SignalHandlerKind,
     target_runner::{PlatformRunner, TargetRunner},
@@ -2083,8 +2080,8 @@ impl DebugCommand {
                         }
                     })?;
 
-                    let description_kind = extract_description(&combined, &combined);
-                    display_description_kind(description_kind, output_format)?;
+                    let description_kind = extract_slice_from_output(&combined, &combined);
+                    display_output_slice(description_kind, output_format)?;
                 } else {
                     let stdout = stdout
                         .map(|path| {
@@ -2111,8 +2108,8 @@ impl DebugCommand {
                         .transpose()?
                         .unwrap_or_default();
 
-                    let description_kind = extract_description(&stdout, &stderr);
-                    display_description_kind(description_kind, output_format)?;
+                    let output_slice = extract_slice_from_output(&stdout, &stderr);
+                    display_output_slice(output_slice, output_format)?;
                 }
             }
         }
@@ -2121,25 +2118,20 @@ impl DebugCommand {
     }
 }
 
-fn extract_description<'a>(stdout: &'a [u8], stderr: &'a [u8]) -> Option<DescriptionKind<'a>> {
-    // The execution result is a generic one.
-    heuristic_extract_description(
-        ExecutionResult::Fail {
-            abort_status: None,
-            leaked: false,
-        },
-        Some(stdout),
-        Some(stderr),
-    )
+fn extract_slice_from_output<'a>(
+    stdout: &'a [u8],
+    stderr: &'a [u8],
+) -> Option<TestOutputErrorSlice<'a>> {
+    heuristic_slice_from_output(Some(stdout), Some(stderr))
 }
 
-fn display_description_kind(
-    kind: Option<DescriptionKind<'_>>,
+fn display_output_slice(
+    output_slice: Option<TestOutputErrorSlice<'_>>,
     output_format: ExtractOutputFormat,
 ) -> Result<()> {
     match output_format {
         ExtractOutputFormat::Raw => {
-            if let Some(kind) = kind {
+            if let Some(kind) = output_slice {
                 if let Some(out) = kind.combined_subslice() {
                     return std::io::stdout().write_all(out.slice).map_err(|err| {
                         ExpectedError::DebugExtractWriteError {
@@ -2151,15 +2143,12 @@ fn display_description_kind(
             }
         }
         ExtractOutputFormat::JunitDescription => {
-            if let Some(kind) = kind {
-                println!(
-                    "{}",
-                    XmlString::new(kind.display_human().to_string()).as_str()
-                );
+            if let Some(kind) = output_slice {
+                println!("{}", XmlString::new(kind.to_string()).as_str());
             }
         }
         ExtractOutputFormat::Highlight => {
-            if let Some(kind) = kind {
+            if let Some(kind) = output_slice {
                 if let Some(out) = kind.combined_subslice() {
                     let end = highlight_end(out.slice);
                     return std::io::stdout()

nextest-runner/src/errors.rs

@@ -355,14 +355,24 @@ impl<T: std::error::Error> ErrorList<T> {
         }
     }
 
-    /// Returns a 1 line summary of the error list.
-    pub(crate) fn as_one_line_summary(&self) -> String {
+    /// Returns a short summary of the error list.
+    pub(crate) fn short_message(&self) -> String {
         if self.inner.len() == 1 {
+            // This assumes that the error's `std::error::Error` implementation
+            // provides a short message as its `Display` implementation. That
+            // isn't always true -- in fact, `ErrorList`'s own `Display`
+            // implementation is multi-line -- but it is true in practice for
+            // all the errors we have in place right now. We may want to make
+            // this better in the future.
             format!("{}", self.inner[0])
         } else {
             format!("{} errors occurred {}", self.inner.len(), self.description)
         }
     }
+
+    pub(crate) fn iter(&self) -> impl Iterator<Item = &T> {
+        self.inner.iter()
+    }
 }
 
 impl<T: std::error::Error> fmt::Display for ErrorList<T> {

nextest-runner/src/reporter/aggregator.rs

@@ -3,14 +3,14 @@
 
 //! Metadata management.
 
-use super::TestEvent;
+use super::{TestEvent, UnitErrorDescription, UnitKind};
 use crate::{
     config::{EvaluatableProfile, NextestJunitConfig},
     errors::{DisplayErrorChain, WriteEventError},
     list::TestInstance,
     reporter::TestEventKind,
     runner::{ExecuteStatus, ExecutionDescription, ExecutionResult},
-    test_output::{ChildExecutionResult, ChildOutput},
+    test_output::{ChildExecutionOutput, ChildOutput},
 };
 use camino::Utf8PathBuf;
 use debug_ignore::DebugIgnore;
@@ -150,7 +150,7 @@ impl<'cfg> MetadataJunit<'cfg> {
                         .set_type(ty);
 
                     set_execute_status_props(
-                        rerun,
+                        &rerun.output,
                         // Reruns are always failures.
                         false,
                         junit_store_failure_output,
@@ -176,7 +176,7 @@ impl<'cfg> MetadataJunit<'cfg> {
                     || (junit_store_failure_output && !is_success);
 
                 set_execute_status_props(
-                    main_status,
+                    &main_status.output,
                     is_success,
                     store_stdout_stderr,
                     TestcaseOrRerun::Testcase(&mut testcase),
@@ -297,45 +297,34 @@ impl TestcaseOrRerun<'_> {
 }
 
 fn set_execute_status_props(
-    execute_status: &ExecuteStatus,
+    exec_output: &ChildExecutionOutput,
     is_success: bool,
     store_stdout_stderr: bool,
     mut out: TestcaseOrRerun<'_>,
 ) {
-    match &execute_status.output {
-        ChildExecutionResult::Output { output, errors } => {
-            if !is_success {
-                if let Some(errors) = errors {
-                    // Use the child errors as the message and description.
-                    out.set_message(errors.as_one_line_summary());
-                    out.set_description(DisplayErrorChain::new(errors).to_string());
-                };
-                let description = output.heuristic_extract_description(execute_status.result);
-                if let Some(description) = description {
-                    out.set_description(description.display_human().to_string());
-                }
-            }
+    // Currently we only aggregate test results, so always specify UnitKind::Test.
+    let description = UnitErrorDescription::new(UnitKind::Test, &exec_output);
+    if let Some(errors) = description.all_error_list() {
+        out.set_message(errors.short_message());
+        out.set_description(DisplayErrorChain::new(errors).to_string());
+    }
 
-            if store_stdout_stderr {
-                match output {
-                    ChildOutput::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());
-                        }
+    if !is_success && store_stdout_stderr {
+        if let ChildExecutionOutput::Output { output, .. } = exec_output {
+            match output {
+                ChildOutput::Split(split) => {
+                    if let Some(stdout) = &split.stdout {
+                        out.set_system_out(stdout.as_str_lossy());
                     }
-                    ChildOutput::Combined { output } => {
-                        out.set_system_out(output.as_str_lossy())
-                            .set_system_err("(stdout and stderr are combined)");
+                    if let Some(stderr) = &split.stderr {
+                        out.set_system_err(stderr.as_str_lossy());
                     }
                 }
+                ChildOutput::Combined { output } => {
+                    out.set_system_out(output.as_str_lossy())
+                        .set_system_err("(stdout and stderr are combined)");
+                }
             }
         }
-        ChildExecutionResult::StartError(error) => {
-            out.set_message(format!("Test execution failed: {error}"));
-            out.set_description(DisplayErrorChain::new(error).to_string());
-        }
     }
 }

nextest-runner/src/reporter/displayer.rs

@@ -6,20 +6,20 @@
 //! The main structure in this module is [`TestReporter`].
 
 use super::{
-    helpers::ByteSubslice, structured::StructuredReporter, CancelReason, DescriptionKind,
-    TestEvent, TestEventKind,
+    structured::StructuredReporter, ByteSubslice, CancelReason, TestEvent, TestEventKind,
+    TestOutputErrorSlice, UnitKind,
 };
 use crate::{
     config::{CompiledDefaultFilter, EvaluatableProfile, ScriptId},
     errors::{DisplayErrorChain, WriteEventError},
     helpers::{plural, DisplayScriptInstance, DisplayTestInstance},
     list::{SkipCounts, TestInstance, TestList},
-    reporter::{aggregator::EventAggregator, helpers::highlight_end},
+    reporter::{aggregator::EventAggregator, helpers::highlight_end, UnitErrorDescription},
     runner::{
         AbortStatus, ExecuteStatus, ExecutionDescription, ExecutionResult, ExecutionStatuses,
         FinalRunStats, RetryData, RunStats, RunStatsFailureKind, SetupScriptExecuteStatus,
     },
-    test_output::{ChildExecutionResult, ChildOutput, ChildSingleOutput},
+    test_output::{ChildExecutionOutput, ChildOutput, ChildSingleOutput},
 };
 use bstr::ByteSlice;
 use debug_ignore::DebugIgnore;
@@ -1375,26 +1375,7 @@ impl<'a> TestReporterImpl<'a> {
         writer: &mut dyn Write,
     ) -> io::Result<()> {
         let spec = self.output_spec_for_script(script_id, command, args, run_status);
-
-        match &run_status.output {
-            ChildExecutionResult::Output { output, errors } => {
-                // Show execution failures first so that they show up
-                // immediately after the failure notification.
-                if let Some(errors) = errors {
-                    let error_chain = DisplayErrorChain::new(errors);
-                    writeln!(writer, "{}\n{error_chain}", spec.exec_fail_header)?;
-                }
-
-                self.write_child_output(&spec, output, None, writer)?;
-            }
-
-            ChildExecutionResult::StartError(error) => {
-                let error_chain = DisplayErrorChain::new(error);
-                writeln!(writer, "{}\n{error_chain}", spec.exec_fail_header)?;
-            }
-        }
-
-        writeln!(writer)
+        self.write_child_execution_output(&spec, &run_status.output, writer)
     }
 
     fn write_test_execute_status(
@@ -1405,26 +1386,40 @@ impl<'a> TestReporterImpl<'a> {
         writer: &mut dyn Write,
     ) -> io::Result<()> {
         let spec = self.output_spec_for_test(test_instance, run_status, is_retry);
+        self.write_child_execution_output(&spec, &run_status.output, writer)
+    }
+
+    fn write_child_execution_output(
+        &self,
+        spec: &ChildOutputSpec,
+        exec_output: &ChildExecutionOutput,
+        writer: &mut dyn Write,
+    ) -> io::Result<()> {
+        match exec_output {
+            ChildExecutionOutput::Output {
+                output,
+                // result and errors are captured by desc.
+                result: _,
+                errors: _,
+            } => {
+                let desc = UnitErrorDescription::new(spec.kind, exec_output);
 
-        match &run_status.output {
-            ChildExecutionResult::Output { output, errors } => {
                 // Show execution failures first so that they show up
                 // immediately after the failure notification.
-                if let Some(errors) = errors {
+                if let Some(errors) = desc.exec_fail_error_list() {
                     let error_chain = DisplayErrorChain::new(errors);
                     writeln!(writer, "{}\n{error_chain}", spec.exec_fail_header)?;
                 }
 
-                let description = if self.styles.is_colorized {
-                    output.heuristic_extract_description(run_status.result)
+                let highlight_slice = if self.styles.is_colorized {
+                    desc.output_slice()
                 } else {
                     None
                 };
-                let spec = self.output_spec_for_test(test_instance, run_status, is_retry);
-                self.write_child_output(&spec, output, description, writer)?;
+                self.write_child_output(&spec, output, highlight_slice, writer)?;
             }
 
-            ChildExecutionResult::StartError(error) => {
+            ChildExecutionOutput::StartError(error) => {
                 let error_chain = DisplayErrorChain::new(error);
                 writeln!(writer, "{}\n{error_chain}", spec.exec_fail_header)?;
             }
@@ -1437,7 +1432,7 @@ impl<'a> TestReporterImpl<'a> {
         &self,
         spec: &ChildOutputSpec,
         output: &ChildOutput,
-        description: Option<DescriptionKind<'_>>,
+        highlight_slice: Option<TestOutputErrorSlice<'_>>,
         mut writer: &mut dyn Write,
     ) -> io::Result<()> {
         match output {
@@ -1453,7 +1448,7 @@ impl<'a> TestReporterImpl<'a> {
                         let mut indent_writer = IndentWriter::new(spec.output_indent, writer);
                         self.write_test_single_output_with_description(
                             stdout,
-                            description.and_then(|d| d.stdout_subslice()),
+                            highlight_slice.and_then(|d| d.stdout_subslice()),
                             &mut indent_writer,
                         )?;
                         indent_writer.flush()?;
@@ -1468,7 +1463,7 @@ impl<'a> TestReporterImpl<'a> {
                         let mut indent_writer = IndentWriter::new(spec.output_indent, writer);
                         self.write_test_single_output_with_description(
                             stderr,
-                            description.and_then(|d| d.stderr_subslice()),
+                            highlight_slice.and_then(|d| d.stderr_subslice()),
                             &mut indent_writer,
                         )?;
                         indent_writer.flush()?;
@@ -1482,7 +1477,7 @@ impl<'a> TestReporterImpl<'a> {
                     let mut indent_writer = IndentWriter::new(spec.output_indent, writer);
                     self.write_test_single_output_with_description(
                         output,
-                        description.and_then(|d| d.combined_subslice()),
+                        highlight_slice.and_then(|d| d.combined_subslice()),
                         &mut indent_writer,
                     )?;
                     indent_writer.flush()?;
@@ -1618,6 +1613,7 @@ impl<'a> TestReporterImpl<'a> {
         };
 
         ChildOutputSpec {
+            kind: UnitKind::Test,
             stdout_header,
             stderr_header,
             combined_header,
@@ -1680,6 +1676,7 @@ impl<'a> TestReporterImpl<'a> {
         };
 
         ChildOutputSpec {
+            kind: UnitKind::Script,
             stdout_header,
             stderr_header,
             combined_header,
@@ -1706,6 +1703,7 @@ const RESET_COLOR: &[u8] = b"\x1b[0m";
 /// measurably slow.
 #[derive(Debug)]
 struct ChildOutputSpec {
+    kind: UnitKind,
     stdout_header: String,
     stderr_header: String,
     combined_header: String,