From 2db19f27dafeb3d7c6349363729fbd434f138c87 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 25 Oct 2025 12:51:30 +1100 Subject: [PATCH 1/4] Don't pass `Arc` to `runtest::run` This function doesn't need its own clone of the `Arc`, and can just take a reference instead. --- src/tools/compiletest/src/executor.rs | 2 +- src/tools/compiletest/src/runtest.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/tools/compiletest/src/executor.rs b/src/tools/compiletest/src/executor.rs index c7aca6d1c5aaa..733b977872081 100644 --- a/src/tools/compiletest/src/executor.rs +++ b/src/tools/compiletest/src/executor.rs @@ -224,7 +224,7 @@ impl RunnableTest { fn run(&self, stdout: &dyn ConsoleOut, stderr: &dyn ConsoleOut) { __rust_begin_short_backtrace(|| { crate::runtest::run( - Arc::clone(&self.config), + &self.config, stdout, stderr, &self.testpaths, diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index ff275df9b31c0..bfae1ed370d1c 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -6,7 +6,6 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use std::io::prelude::*; use std::io::{self, BufReader}; use std::process::{Child, Command, ExitStatus, Output, Stdio}; -use std::sync::Arc; use std::{env, fmt, iter, str}; use build_helper::fs::remove_and_create_dir_all; @@ -110,7 +109,7 @@ fn dylib_name(name: &str) -> String { } pub fn run( - config: Arc, + config: &Config, stdout: &dyn ConsoleOut, stderr: &dyn ConsoleOut, testpaths: &TestPaths, From f988679d23bae53633e58d2640ed63ca67b2ed9c Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 25 Oct 2025 12:49:50 +1100 Subject: [PATCH 2/4] Rename `RunnableTest` to `TestThreadArgs` and inline its methods --- src/tools/compiletest/src/executor.rs | 66 +++++++++++++-------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/tools/compiletest/src/executor.rs b/src/tools/compiletest/src/executor.rs index 733b977872081..06c83fbc9d074 100644 --- a/src/tools/compiletest/src/executor.rs +++ b/src/tools/compiletest/src/executor.rs @@ -107,23 +107,33 @@ fn spawn_test_thread( return None; } - let runnable_test = RunnableTest::new(test); + let args = TestThreadArgs { + config: Arc::clone(&test.config), + testpaths: test.testpaths.clone(), + revision: test.revision.clone(), + }; let should_panic = test.desc.should_panic; - let run_test = move || run_test_inner(id, should_panic, runnable_test, completion_tx); + let run_test = move || test_thread_main(id, should_panic, args, completion_tx); let thread_builder = thread::Builder::new().name(test.desc.name.clone()); let join_handle = thread_builder.spawn(run_test).unwrap(); Some(join_handle) } +struct TestThreadArgs { + config: Arc, + testpaths: TestPaths, + revision: Option, +} + /// Runs a single test, within the dedicated thread spawned by the caller. -fn run_test_inner( +fn test_thread_main( id: TestId, should_panic: ShouldPanic, - runnable_test: RunnableTest, + args: TestThreadArgs, completion_sender: mpsc::Sender, ) { - let capture = CaptureKind::for_config(&runnable_test.config); + let capture = CaptureKind::for_config(&args.config); // Install a panic-capture buffer for use by the custom panic hook. if capture.should_set_panic_hook() { @@ -133,7 +143,24 @@ fn run_test_inner( let stdout = capture.stdout(); let stderr = capture.stderr(); - let panic_payload = panic::catch_unwind(move || runnable_test.run(stdout, stderr)).err(); + // Run the test, catching any panics so that we can gracefully report + // failure (or success). + // + // FIXME(Zalathar): Ideally we would report test failures with `Result`, + // and use panics only for bugs within compiletest itself, but that would + // require a major overhaul of error handling in the test runners. + let panic_payload = panic::catch_unwind(|| { + __rust_begin_short_backtrace(|| { + crate::runtest::run( + &args.config, + stdout, + stderr, + &args.testpaths, + args.revision.as_deref(), + ); + }); + }) + .err(); if let Some(panic_buf) = panic_hook::take_capture_buf() { let panic_buf = panic_buf.lock().unwrap_or_else(|e| e.into_inner()); @@ -207,33 +234,6 @@ impl CaptureKind { #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] struct TestId(usize); -struct RunnableTest { - config: Arc, - testpaths: TestPaths, - revision: Option, -} - -impl RunnableTest { - fn new(test: &CollectedTest) -> Self { - let config = Arc::clone(&test.config); - let testpaths = test.testpaths.clone(); - let revision = test.revision.clone(); - Self { config, testpaths, revision } - } - - fn run(&self, stdout: &dyn ConsoleOut, stderr: &dyn ConsoleOut) { - __rust_begin_short_backtrace(|| { - crate::runtest::run( - &self.config, - stdout, - stderr, - &self.testpaths, - self.revision.as_deref(), - ); - }); - } -} - /// Fixed frame used to clean the backtrace with `RUST_BACKTRACE=1`. #[inline(never)] fn __rust_begin_short_backtrace T>(f: F) -> T { From 665dbb373923cde80eddce31d653abd726b071a5 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 25 Oct 2025 13:02:57 +1100 Subject: [PATCH 3/4] Move all other test thread args into `TestThreadArgs` --- src/tools/compiletest/src/executor.rs | 30 ++++++++++++++------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/tools/compiletest/src/executor.rs b/src/tools/compiletest/src/executor.rs index 06c83fbc9d074..d6dbdbf8639e6 100644 --- a/src/tools/compiletest/src/executor.rs +++ b/src/tools/compiletest/src/executor.rs @@ -98,41 +98,42 @@ pub(crate) fn run_tests(config: &Config, tests: Vec) -> bool { fn spawn_test_thread( id: TestId, test: &CollectedTest, - completion_tx: mpsc::Sender, + completion_sender: mpsc::Sender, ) -> Option> { if test.desc.ignore && !test.config.run_ignored { - completion_tx + completion_sender .send(TestCompletion { id, outcome: TestOutcome::Ignored, stdout: None }) .unwrap(); return None; } let args = TestThreadArgs { + id, config: Arc::clone(&test.config), testpaths: test.testpaths.clone(), revision: test.revision.clone(), + should_panic: test.desc.should_panic, + completion_sender, }; - let should_panic = test.desc.should_panic; - let run_test = move || test_thread_main(id, should_panic, args, completion_tx); - let thread_builder = thread::Builder::new().name(test.desc.name.clone()); - let join_handle = thread_builder.spawn(run_test).unwrap(); + let join_handle = thread_builder.spawn(move || test_thread_main(args)).unwrap(); Some(join_handle) } +/// All of the owned data needed by `test_thread_main`. struct TestThreadArgs { + id: TestId, + config: Arc, testpaths: TestPaths, revision: Option, + should_panic: ShouldPanic, + + completion_sender: mpsc::Sender, } /// Runs a single test, within the dedicated thread spawned by the caller. -fn test_thread_main( - id: TestId, - should_panic: ShouldPanic, - args: TestThreadArgs, - completion_sender: mpsc::Sender, -) { +fn test_thread_main(args: TestThreadArgs) { let capture = CaptureKind::for_config(&args.config); // Install a panic-capture buffer for use by the custom panic hook. @@ -168,7 +169,8 @@ fn test_thread_main( write!(stderr, "{panic_buf}"); } - let outcome = match (should_panic, panic_payload) { + // Interpret the presence/absence of a panic as test failure/success. + let outcome = match (args.should_panic, panic_payload) { (ShouldPanic::No, None) | (ShouldPanic::Yes, Some(_)) => TestOutcome::Succeeded, (ShouldPanic::No, Some(_)) => TestOutcome::Failed { message: None }, (ShouldPanic::Yes, None) => { @@ -177,7 +179,7 @@ fn test_thread_main( }; let stdout = capture.into_inner(); - completion_sender.send(TestCompletion { id, outcome, stdout }).unwrap(); + args.completion_sender.send(TestCompletion { id: args.id, outcome, stdout }).unwrap(); } enum CaptureKind { From bb20367178be12ba9afb556104df4f55dad88d40 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Sat, 25 Oct 2025 13:35:58 +1100 Subject: [PATCH 4/4] Rename `ShouldPanic` to `ShouldFail` The old name was a holdover from libtest, but in compiletest we only use it for `//@ should-fail` tests, which are tests of compiletest itself. --- src/tools/compiletest/src/directives.rs | 12 +++++------ src/tools/compiletest/src/directives/tests.rs | 6 +++--- src/tools/compiletest/src/executor.rs | 21 ++++++++++--------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/tools/compiletest/src/directives.rs b/src/tools/compiletest/src/directives.rs index 51eac58c971eb..0318ed2b3d119 100644 --- a/src/tools/compiletest/src/directives.rs +++ b/src/tools/compiletest/src/directives.rs @@ -18,7 +18,7 @@ use crate::directives::line::{DirectiveLine, line_directive}; use crate::directives::needs::CachedNeedsConditions; use crate::edition::{Edition, parse_edition}; use crate::errors::ErrorKind; -use crate::executor::{CollectedTestDesc, ShouldPanic}; +use crate::executor::{CollectedTestDesc, ShouldFail}; use crate::util::static_regex; use crate::{fatal, help}; @@ -1366,10 +1366,10 @@ pub(crate) fn make_test_description( // The `should-fail` annotation doesn't apply to pretty tests, // since we run the pretty printer across all tests by default. // If desired, we could add a `should-fail-pretty` annotation. - let should_panic = match config.mode { - TestMode::Pretty => ShouldPanic::No, - _ if should_fail => ShouldPanic::Yes, - _ => ShouldPanic::No, + let should_fail = if should_fail && config.mode != TestMode::Pretty { + ShouldFail::Yes + } else { + ShouldFail::No }; CollectedTestDesc { @@ -1377,7 +1377,7 @@ pub(crate) fn make_test_description( filterable_path: filterable_path.to_owned(), ignore, ignore_message, - should_panic, + should_fail, } } diff --git a/src/tools/compiletest/src/directives/tests.rs b/src/tools/compiletest/src/directives/tests.rs index 98249e69601be..0bf2bcd3af434 100644 --- a/src/tools/compiletest/src/directives/tests.rs +++ b/src/tools/compiletest/src/directives/tests.rs @@ -7,7 +7,7 @@ use crate::directives::{ extract_llvm_version, extract_version_range, iter_directives, line_directive, parse_edition, parse_normalize_rule, }; -use crate::executor::{CollectedTestDesc, ShouldPanic}; +use crate::executor::{CollectedTestDesc, ShouldFail}; fn make_test_description( config: &Config, @@ -247,9 +247,9 @@ fn should_fail() { let p = Utf8Path::new("a.rs"); let d = make_test_description(&config, tn.clone(), p, p, "", None); - assert_eq!(d.should_panic, ShouldPanic::No); + assert_eq!(d.should_fail, ShouldFail::No); let d = make_test_description(&config, tn, p, p, "//@ should-fail", None); - assert_eq!(d.should_panic, ShouldPanic::Yes); + assert_eq!(d.should_fail, ShouldFail::Yes); } #[test] diff --git a/src/tools/compiletest/src/executor.rs b/src/tools/compiletest/src/executor.rs index d6dbdbf8639e6..5a2136c55b05e 100644 --- a/src/tools/compiletest/src/executor.rs +++ b/src/tools/compiletest/src/executor.rs @@ -112,7 +112,7 @@ fn spawn_test_thread( config: Arc::clone(&test.config), testpaths: test.testpaths.clone(), revision: test.revision.clone(), - should_panic: test.desc.should_panic, + should_fail: test.desc.should_fail, completion_sender, }; let thread_builder = thread::Builder::new().name(test.desc.name.clone()); @@ -127,7 +127,7 @@ struct TestThreadArgs { config: Arc, testpaths: TestPaths, revision: Option, - should_panic: ShouldPanic, + should_fail: ShouldFail, completion_sender: mpsc::Sender, } @@ -170,11 +170,11 @@ fn test_thread_main(args: TestThreadArgs) { } // Interpret the presence/absence of a panic as test failure/success. - let outcome = match (args.should_panic, panic_payload) { - (ShouldPanic::No, None) | (ShouldPanic::Yes, Some(_)) => TestOutcome::Succeeded, - (ShouldPanic::No, Some(_)) => TestOutcome::Failed { message: None }, - (ShouldPanic::Yes, None) => { - TestOutcome::Failed { message: Some("test did not panic as expected") } + let outcome = match (args.should_fail, panic_payload) { + (ShouldFail::No, None) | (ShouldFail::Yes, Some(_)) => TestOutcome::Succeeded, + (ShouldFail::No, Some(_)) => TestOutcome::Failed { message: None }, + (ShouldFail::Yes, None) => { + TestOutcome::Failed { message: Some("`//@ should-fail` test did not fail as expected") } } }; @@ -338,7 +338,7 @@ pub(crate) struct CollectedTestDesc { pub(crate) filterable_path: Utf8PathBuf, pub(crate) ignore: bool, pub(crate) ignore_message: Option>, - pub(crate) should_panic: ShouldPanic, + pub(crate) should_fail: ShouldFail, } /// Whether console output should be colored or not. @@ -350,9 +350,10 @@ pub enum ColorConfig { NeverColor, } -/// Whether test is expected to panic or not. +/// Tests with `//@ should-fail` are tests of compiletest itself, and should +/// be reported as successful if and only if they would have _failed_. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] -pub(crate) enum ShouldPanic { +pub(crate) enum ShouldFail { No, Yes, }