Skip to content

Commit

Permalink
[nextest-runner] make Windows job object termination clearer
Browse files Browse the repository at this point in the history
In case of a Ctrl-C grace period expiring, we should make it clear to users
what's happening rather than just printing out "FAIL".

Also make a slight formatting tweak.
  • Loading branch information
sunshowers committed Dec 17, 2024
1 parent c612aea commit 69979ef
Show file tree
Hide file tree
Showing 9 changed files with 262 additions and 161 deletions.
6 changes: 3 additions & 3 deletions nextest-runner/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use crate::{
cargo_config::{TargetTriple, TargetTripleSource},
config::{ConfigExperimental, CustomTestGroup, ScriptId, TestGroup},
helpers::{display_exit_status, dylib_path_envvar},
helpers::{display_exited_with, dylib_path_envvar},
redact::Redactor,
reuse_build::{ArchiveFormat, ArchiveStep},
target_runner::PlatformRunnerSource,
Expand Down Expand Up @@ -873,9 +873,9 @@ pub enum CreateTestListError {

/// Running a command to gather the list of tests failed failed with a non-zero exit code.
#[error(
"for `{binary_id}`, command `{}` exited with {}\n--- stdout:\n{}\n--- stderr:\n{}\n---",
"for `{binary_id}`, command `{}` {}\n--- stdout:\n{}\n--- stderr:\n{}\n---",
shell_words::join(command),
display_exit_status(*exit_status),
display_exited_with(*exit_status),
String::from_utf8_lossy(stdout),
String::from_utf8_lossy(stderr),
)]
Expand Down
17 changes: 10 additions & 7 deletions nextest-runner/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,36 +296,39 @@ impl fmt::Display for FormattedDuration {
}
}

pub(crate) fn display_exit_status(exit_status: ExitStatus) -> String {
// "exited with"/"terminated via"
pub(crate) fn display_exited_with(exit_status: ExitStatus) -> String {
match AbortStatus::extract(exit_status) {
Some(abort_status) => display_abort_status(abort_status),
None => match exit_status.code() {
Some(code) => format!("exit code {}", code),
None => "an unknown error".to_owned(),
Some(code) => format!("exited with exit code {}", code),
None => "exited with an unknown error".to_owned(),
},
}
}

/// Display the abort status.
/// Displays the abort status.
pub(crate) fn display_abort_status(abort_status: AbortStatus) -> String {
match abort_status {
#[cfg(unix)]
AbortStatus::UnixSignal(sig) => match crate::helpers::signal_str(sig) {
Some(s) => {
format!("signal {sig} (SIG{s})")
format!("aborted with signal {sig} (SIG{s})")
}
None => {
format!("signal {sig}")
format!("aborted with signal {sig}")
}
},
#[cfg(windows)]
AbortStatus::WindowsNtStatus(nt_status) => {
format!(
"code {}",
"aborted with code {}",
// TODO: pass down a style here
crate::helpers::display_nt_status(nt_status, Style::new())
)
}
#[cfg(windows)]
AbortStatus::JobObject => "terminated via job object".to_string(),
}
}

Expand Down
58 changes: 38 additions & 20 deletions nextest-runner/src/reporter/displayer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1319,11 +1319,11 @@ impl<'a> TestReporterImpl<'a> {
// On Windows, also print out the exception if available.
#[cfg(windows)]
if let ExecutionResult::Fail {
abort_status: Some(AbortStatus::WindowsNtStatus(nt_status)),
abort_status: Some(abort_status),
leaked: _,
} = last_status.result
{
write_windows_message_line(nt_status, &self.styles, writer)?;
write_windows_message_line(abort_status, &self.styles, writer)?;
}

Ok(())
Expand Down Expand Up @@ -1393,7 +1393,7 @@ impl<'a> TestReporterImpl<'a> {
// On Windows, also print out the exception if available.
#[cfg(windows)]
if let ExecutionResult::Fail {
abort_status: Some(AbortStatus::WindowsNtStatus(nt_status)),
abort_status: Some(abort_status),
leaked: _,
} = last_status.result
{
Expand Down Expand Up @@ -2453,7 +2453,7 @@ fn status_str(result: ExecutionResult) -> Cow<'static, str> {
},
#[cfg(windows)]
ExecutionResult::Fail {
abort_status: Some(AbortStatus::WindowsNtStatus(_)),
abort_status: Some(AbortStatus::WindowsNtStatus(_)) | Some(AbortStatus::JobObject),
leaked: _,
} => {
// Going to print out the full error message on the following line -- just "ABORT" will
Expand Down Expand Up @@ -2488,7 +2488,7 @@ fn short_status_str(result: ExecutionResult) -> Cow<'static, str> {
},
#[cfg(windows)]
ExecutionResult::Fail {
abort_status: Some(AbortStatus::WindowsNtStatus(_)),
abort_status: Some(AbortStatus::WindowsNtStatus(_)) | Some(AbortStatus::JobObject),
leaked: _,
} => {
// Going to print out the full error message on the following line -- just "ABORT" will
Expand All @@ -2508,21 +2508,35 @@ fn short_status_str(result: ExecutionResult) -> Cow<'static, str> {

#[cfg(windows)]
fn write_windows_message_line(
nt_status: windows_sys::Win32::Foundation::NTSTATUS,
status: AbortStatus,
styles: &Styles,
writer: &mut dyn Write,
) -> io::Result<()> {
// Note that display_nt_status ensures that codes are aligned properly. For subsequent lines,
// use an indented displayer with 25 spaces (ensuring that message lines are aligned).
const INDENT: &str = " - ";
let mut indented = IndentWriter::new_skip_initial(INDENT, writer);
writeln!(
indented,
"{:>12} {}",
"with code".style(styles.fail),
crate::helpers::display_nt_status(nt_status, styles.count)
)?;
indented.flush()
match abort_status {
AbortStatus::WindowsNtStatus(nt_status) => {
// Note that display_nt_status ensures that codes are aligned properly. For subsequent lines,
// use an indented displayer with 25 spaces (ensuring that message lines are aligned).
const INDENT: &str = " - ";
let mut indented = IndentWriter::new_skip_initial(INDENT, writer);
writeln!(
indented,
"{:>12} {} {}",
"",
"with code".style(styles.fail),
crate::helpers::display_nt_status(nt_status, styles.count)
)?;
indented.flush()
}
AbortStatus::JobObject => {
write!(
writer,
"{:>12} {} via {}",
"",
"terminated".style(self.styles.fail),
"job object".style(self.styles.count),
)?;
}
}
}

fn write_final_warnings(
Expand Down Expand Up @@ -3737,15 +3751,19 @@ mod windows_tests {
SetThreadUILanguage(0x0409);
}

insta::assert_snapshot!("ctrl_c_code", to_message_line(STATUS_CONTROL_C_EXIT));
insta::assert_snapshot!(
"ctrl_c_code",
to_message_line(AbortStatus::WindowsNtStatus(STATUS_CONTROL_C_EXIT))
);
insta::assert_snapshot!(
"stack_violation_code",
to_message_line(STATUS_CONTROL_STACK_VIOLATION),
to_message_line(AbortStatus::WindowsNtStatus(STATUS_CONTROL_STACK_VIOLATION)),
);
insta::assert_snapshot!("job_object", to_message_line(AbortStatus::JobObject));
}

#[track_caller]
fn to_message_line(status: NTSTATUS) -> String {
fn to_message_line(status: AbortStatus) -> String {
let mut buf = Vec::new();
write_windows_message_line(status, &Styles::default(), &mut buf).unwrap();
String::from_utf8(buf).unwrap()
Expand Down
22 changes: 2 additions & 20 deletions nextest-runner/src/reporter/error_description.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use super::events::{AbortStatus, ExecutionResult, UnitKind};
use crate::{
errors::{ChildError, ChildStartError, ErrorList},
helpers::display_abort_status,
test_output::{ChildExecutionOutput, ChildOutput},
};
use bstr::ByteSlice;
Expand Down Expand Up @@ -152,26 +153,7 @@ struct UnitAbortDescription {

impl fmt::Display for UnitAbortDescription {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "process aborted")?;
match self.status {
#[cfg(unix)]
AbortStatus::UnixSignal(sig) => {
let signal_str = crate::helpers::signal_str(sig)
.map(|signal_str| format!("SIG{signal_str}"))
.unwrap_or_else(|| sig.to_string());
write!(f, " with signal {signal_str}")?;
}
#[cfg(windows)]
AbortStatus::WindowsNtStatus(exception) => {
write!(
f,
" with code {}",
// TODO: pass in bold style (probably need to not use
// fmt::Display)
crate::helpers::display_nt_status(exception, owo_colors::Style::new())
)?;
}
}
write!(f, "process {}", display_abort_status(self.status))?;
if self.leaked {
write!(f, ", and also leaked handles")?;
}
Expand Down
6 changes: 5 additions & 1 deletion nextest-runner/src/reporter/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -741,7 +741,7 @@ pub enum ExecutionResult {
},
/// An error occurred while executing the test.
ExecFail,
/// The test was terminated due to timeout.
/// The test was terminated due to a timeout.
Timeout,
}

Expand Down Expand Up @@ -769,6 +769,10 @@ pub enum AbortStatus {
/// The test was determined to have aborted because the high bit was set on Windows.
#[cfg(windows)]
WindowsNtStatus(windows_sys::Win32::Foundation::NTSTATUS),

/// The test was terminated via job object on Windows.
#[cfg(windows)]
JobObject,
}

impl AbortStatus {
Expand Down
Loading

0 comments on commit 69979ef

Please sign in to comment.