Rollup merge of #148088 - Zalathar:test-thread, r=jieyouxu

compiletest: Simplify passing arguments to spawned test threads

The current code structure was heavily influenced by wanting to match the libtest executor as closely as possible.

Now that the libtest executor has been removed, we can get rid of some complexity that no longer serves a purpose in the new executor.

---

The renaming of `ShouldPanic` is only semi-related, but I included it here because it's small, and as a separate PR it would have conflicted with this one.

r? jieyouxu

src/tools/compiletest/src/directives.rs

@@ -18,7 +18,7 @@
 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 @@ macro_rules! decision {
     // 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 @@ macro_rules! decision {
         filterable_path: filterable_path.to_owned(),
         ignore,
         ignore_message,
-        should_panic,
+        should_fail,
     }
 }
 

src/tools/compiletest/src/directives/tests.rs

@@ -7,7 +7,7 @@
     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]

src/tools/compiletest/src/executor.rs

@@ -98,32 +98,43 @@ pub(crate) fn run_tests(config: &Config, tests: Vec<CollectedTest>) -> bool {
 fn spawn_test_thread(
     id: TestId,
     test: &CollectedTest,
-    completion_tx: mpsc::Sender<TestCompletion>,
+    completion_sender: mpsc::Sender<TestCompletion>,
 ) -> Option<thread::JoinHandle<()>> {
     if test.desc.ignore && !test.config.run_ignored {
-        completion_tx
+        completion_sender
             .send(TestCompletion { id, outcome: TestOutcome::Ignored, stdout: None })
             .unwrap();
         return None;
     }
 
-    let runnable_test = RunnableTest::new(test);
-    let should_panic = test.desc.should_panic;
-    let run_test = move || run_test_inner(id, should_panic, runnable_test, completion_tx);
-
+    let args = TestThreadArgs {
+        id,
+        config: Arc::clone(&test.config),
+        testpaths: test.testpaths.clone(),
+        revision: test.revision.clone(),
+        should_fail: test.desc.should_fail,
+        completion_sender,
+    };
     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)
 }
 
-/// Runs a single test, within the dedicated thread spawned by the caller.
-fn run_test_inner(
+/// All of the owned data needed by `test_thread_main`.
+struct TestThreadArgs {
     id: TestId,
-    should_panic: ShouldPanic,
-    runnable_test: RunnableTest,
+
+    config: Arc<Config>,
+    testpaths: TestPaths,
+    revision: Option<String>,
+    should_fail: ShouldFail,
+
     completion_sender: mpsc::Sender<TestCompletion>,
-) {
-    let capture = CaptureKind::for_config(&runnable_test.config);
+}
+
+/// Runs a single test, within the dedicated thread spawned by the caller.
+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.
     if capture.should_set_panic_hook() {
@@ -133,7 +144,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());
@@ -141,16 +169,17 @@ fn run_test_inner(
         write!(stderr, "{panic_buf}");
     }
 
-    let outcome = match (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") }
+    // Interpret the presence/absence of a panic as test failure/success.
+    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") }
         }
     };
 
     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 {
@@ -207,33 +236,6 @@ fn into_inner(self) -> Option<Vec<u8>> {
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
 struct TestId(usize);
 
-struct RunnableTest {
-    config: Arc<Config>,
-    testpaths: TestPaths,
-    revision: Option<String>,
-}
-
-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(
-                Arc::clone(&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: FnOnce() -> T>(f: F) -> T {
@@ -336,7 +338,7 @@ pub(crate) struct CollectedTestDesc {
     pub(crate) filterable_path: Utf8PathBuf,
     pub(crate) ignore: bool,
     pub(crate) ignore_message: Option<Cow<'static, str>>,
-    pub(crate) should_panic: ShouldPanic,
+    pub(crate) should_fail: ShouldFail,
 }
 
 /// Whether console output should be colored or not.
@@ -348,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,
 }

src/tools/compiletest/src/runtest.rs

@@ -6,7 +6,6 @@
 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: &Config,
     stdout: &dyn ConsoleOut,
     stderr: &dyn ConsoleOut,
     testpaths: &TestPaths,