Skip to content

Commit

Permalink
[nextest-runner] improve data model for child error management
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@sunshowers
sunshowers committed Dec 1, 2024
1 parent 866587b commit 4633be8
Show file tree
Hide file tree
Showing 13 changed files with 755 additions and 636 deletions.
47 changes: 18 additions & 29 deletions cargo-nextest/src/dispatch.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
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},
Expand Down Expand Up @@ -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| {
Expand All @@ -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)?;
}
}
}
Expand All @@ -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>> {
TestOutputErrorSlice::heuristic_extract(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 {
Expand All @@ -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()
Expand Down
14 changes: 12 additions & 2 deletions nextest-runner/src/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
57 changes: 23 additions & 34 deletions nextest-runner/src/reporter/aggregator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down Expand Up @@ -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());
}
}
}
70 changes: 34 additions & 36 deletions nextest-runner/src/reporter/displayer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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)?;
}
Expand All @@ -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 {
Expand All @@ -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()?;
Expand All @@ -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()?;
Expand All @@ -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()?;
Expand Down Expand Up @@ -1618,6 +1613,7 @@ impl<'a> TestReporterImpl<'a> {
};

ChildOutputSpec {
kind: UnitKind::Test,
stdout_header,
stderr_header,
combined_header,
Expand Down Expand Up @@ -1680,6 +1676,7 @@ impl<'a> TestReporterImpl<'a> {
};

ChildOutputSpec {
kind: UnitKind::Script,
stdout_header,
stderr_header,
combined_header,
Expand All @@ -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,
Expand Down
Loading

0 comments on commit 4633be8

Please sign in to comment.