Skip to content

Commit

Permalink
Auto merge of #120326 - tmandry:abort-in-tests, r=cuviper
Browse files Browse the repository at this point in the history
Actually abort in -Zpanic-abort-tests

When a test fails in panic=abort, it can be useful to have a debugger or other tooling hook into the `abort()` call for debugging. Doing this some other way would require it to hard code details of Rust's panic machinery.

There's no reason we couldn't have done this in the first place; using a single exit code for "success" or "failed" was just simpler. Now we are aware of the special exit codes for posix and windows platforms, logging a special error if an unrecognized code is used on those platforms, and falling back to just "failure" on other platforms.

This continues to account for `#[should_panic]` inside the test process itself, so there's no risk of misinterpreting a random call to `abort()` as an expected panic. Any exit code besides `TR_OK` is logged as a test failure.

As an added benefit, this would allow us to support panic=immediate_abort (but not `#[should_panic]`), without noise about unexpected exit codes when a test fails.
  • Loading branch information
bors committed Feb 6, 2024
2 parents f3b9d47 + 48f664b commit ff95e52
Show file tree
Hide file tree
Showing 10 changed files with 38 additions and 44 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5321,6 +5321,7 @@ version = "0.0.0"
dependencies = [
"core",
"getopts",
"libc",
"panic_abort",
"panic_unwind",
"std",
Expand Down
1 change: 1 addition & 0 deletions library/test/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ std = { path = "../std" }
core = { path = "../core" }
panic_unwind = { path = "../panic_unwind" }
panic_abort = { path = "../panic_abort" }
libc = { version = "0.2.150", default-features = false }
20 changes: 0 additions & 20 deletions library/test/src/helpers/exit_code.rs

This file was deleted.

1 change: 0 additions & 1 deletion library/test/src/helpers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@
//! but used in `libtest`.

pub mod concurrency;
pub mod exit_code;
pub mod metrics;
pub mod shuffle;
15 changes: 2 additions & 13 deletions library/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ mod tests;
use core::any::Any;
use event::{CompletedTest, TestEvent};
use helpers::concurrency::get_concurrency;
use helpers::exit_code::get_exit_code;
use helpers::shuffle::{get_shuffle_seed, shuffle_tests};
use options::RunStrategy;
use test_result::*;
Expand Down Expand Up @@ -712,17 +711,7 @@ fn spawn_test_subprocess(
formatters::write_stderr_delimiter(&mut test_output, &desc.name);
test_output.extend_from_slice(&stderr);

let result = match (|| -> Result<TestResult, String> {
let exit_code = get_exit_code(status)?;
Ok(get_result_from_exit_code(&desc, exit_code, &time_opts, &exec_time))
})() {
Ok(r) => r,
Err(e) => {
write!(&mut test_output, "Unexpected error: {e}").unwrap();
TrFailed
}
};

let result = get_result_from_exit_code(&desc, status, &time_opts, &exec_time);
(result, test_output, exec_time)
})();

Expand Down Expand Up @@ -751,7 +740,7 @@ fn run_test_in_spawned_subprocess(desc: TestDesc, runnable_test: RunnableTest) -
if let TrOk = test_result {
process::exit(test_result::TR_OK);
} else {
process::exit(test_result::TR_FAILED);
process::abort();
}
});
let record_result2 = record_result.clone();
Expand Down
36 changes: 29 additions & 7 deletions library/test/src/test_result.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
use std::any::Any;
use std::process::ExitStatus;

#[cfg(unix)]
use std::os::unix::process::ExitStatusExt;

use super::bench::BenchSamples;
use super::options::ShouldPanic;
Expand All @@ -7,11 +11,15 @@ use super::types::TestDesc;

pub use self::TestResult::*;

// Return codes for secondary process.
// Return code for secondary process.
// Start somewhere other than 0 so we know the return code means what we think
// it means.
pub const TR_OK: i32 = 50;
pub const TR_FAILED: i32 = 51;

// On Windows we use __fastfail to abort, which is documented to use this
// exception code.
#[cfg(windows)]
const STATUS_ABORTED: i32 = 0xC0000409u32 as i32;

#[derive(Debug, Clone, PartialEq)]
pub enum TestResult {
Expand Down Expand Up @@ -81,14 +89,28 @@ pub fn calc_result<'a>(
/// Creates a `TestResult` depending on the exit code of test subprocess.
pub fn get_result_from_exit_code(
desc: &TestDesc,
code: i32,
status: ExitStatus,
time_opts: &Option<time::TestTimeOptions>,
exec_time: &Option<time::TestExecTime>,
) -> TestResult {
let result = match code {
TR_OK => TestResult::TrOk,
TR_FAILED => TestResult::TrFailed,
_ => TestResult::TrFailedMsg(format!("got unexpected return code {code}")),
let result = match status.code() {
Some(TR_OK) => TestResult::TrOk,
#[cfg(windows)]
Some(STATUS_ABORTED) => TestResult::TrFailed,
#[cfg(unix)]
None => match status.signal() {
Some(libc::SIGABRT) => TestResult::TrFailed,
Some(signal) => {
TestResult::TrFailedMsg(format!("child process exited with signal {signal}"))
}
None => unreachable!("status.code() returned None but status.signal() was None"),
},
#[cfg(not(unix))]
None => TestResult::TrFailedMsg(format!("unknown return code")),
#[cfg(any(windows, unix))]
Some(code) => TestResult::TrFailedMsg(format!("got unexpected return code {code}")),
#[cfg(not(any(windows, unix)))]
Some(_) => TestResult::TrFailed,
};

// If test is already failed (or allowed to fail), do not change the result.
Expand Down
1 change: 1 addition & 0 deletions tests/ui/test-attrs/test-panic-abort-nocapture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// exec-env:RUST_BACKTRACE=0
// normalize-stdout-test "finished in \d+\.\d+s" -> "finished in $$TIME"

// ignore-android #120567
// ignore-wasm no panic or subprocess support
// ignore-emscripten no panic or subprocess support
// ignore-sgx no subprocess support
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/test-attrs/test-panic-abort-nocapture.run.stderr
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
thread 'main' panicked at $DIR/test-panic-abort-nocapture.rs:33:5:
thread 'main' panicked at $DIR/test-panic-abort-nocapture.rs:34:5:
assertion `left == right` failed
left: 2
right: 4
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at $DIR/test-panic-abort-nocapture.rs:27:5:
thread 'main' panicked at $DIR/test-panic-abort-nocapture.rs:28:5:
assertion `left == right` failed
left: 2
right: 4
Expand Down
1 change: 1 addition & 0 deletions tests/ui/test-attrs/test-panic-abort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
// exec-env:RUST_BACKTRACE=0
// normalize-stdout-test "finished in \d+\.\d+s" -> "finished in $$TIME"

// ignore-android #120567
// ignore-wasm no panic or subprocess support
// ignore-emscripten no panic or subprocess support
// ignore-sgx no subprocess support
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/test-attrs/test-panic-abort.run.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ hello, world
testing123
---- it_fails stderr ----
testing321
thread 'main' panicked at $DIR/test-panic-abort.rs:38:5:
thread 'main' panicked at $DIR/test-panic-abort.rs:39:5:
assertion `left == right` failed
left: 2
right: 5
Expand Down

0 comments on commit ff95e52

Please sign in to comment.