Rollup merge of #103681 - RalfJung:libtest-thread, r=thomcc

libtest: run all tests in their own thread, if supported by the host

This reverts the threading changes of #56243, which made it so that with `-j1`, the test harness does not spawn any threads. Those changes were done to enable Miri to run the test harness, but Miri supports threads nowadays, so this is no longer needed. Using a thread for each test is useful because the thread's name can be set to the test's name which makes panic messages consistent between `-j1` and `-j2` runs and also a bit more readable.

I did not revert the HashMap changes of #56243; using a deterministic map seems fine for the test harness and the more deterministic testing is the better.

Fixes #59122
Fixes #70492

library/test/src/lib.rs

@@ -40,7 +40,7 @@ pub mod test {
         cli::{parse_opts, TestOpts},
         filter_tests,
         helpers::metrics::{Metric, MetricMap},
-        options::{Concurrent, Options, RunIgnored, RunStrategy, ShouldPanic},
+        options::{Options, RunIgnored, RunStrategy, ShouldPanic},
         run_test, test_main, test_main_static,
         test_result::{TestResult, TrFailed, TrFailedMsg, TrIgnored, TrOk},
         time::{TestExecTime, TestTimeOptions},
@@ -85,7 +85,7 @@ 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::{Concurrent, RunStrategy};
+use options::RunStrategy;
 use test_result::*;
 use time::TestExecTime;
 
@@ -267,6 +267,19 @@ where
         join_handle: Option<thread::JoinHandle<()>>,
     }
 
+    impl RunningTest {
+        fn join(self, completed_test: &mut CompletedTest) {
+            if let Some(join_handle) = self.join_handle {
+                if let Err(_) = join_handle.join() {
+                    if let TrOk = completed_test.result {
+                        completed_test.result =
+                            TrFailedMsg("panicked after reporting success".to_string());
+                    }
+                }
+            }
+        }
+    }
+
     // Use a deterministic hasher
     type TestMap =
         HashMap<TestId, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
@@ -366,10 +379,10 @@ where
             let (id, test) = remaining.pop_front().unwrap();
             let event = TestEvent::TeWait(test.desc.clone());
             notify_about_test_event(event)?;
-            let join_handle =
-                run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone(), Concurrent::No);
-            assert!(join_handle.is_none());
-            let completed_test = rx.recv().unwrap();
+            let join_handle = run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone());
+            // Wait for the test to complete.
+            let mut completed_test = rx.recv().unwrap();
+            RunningTest { join_handle }.join(&mut completed_test);
 
             let event = TestEvent::TeResult(completed_test);
             notify_about_test_event(event)?;
@@ -383,15 +396,8 @@ where
 
                 let event = TestEvent::TeWait(desc.clone());
                 notify_about_test_event(event)?; //here no pad
-                let join_handle = run_test(
-                    opts,
-                    !opts.run_tests,
-                    id,
-                    test,
-                    run_strategy,
-                    tx.clone(),
-                    Concurrent::Yes,
-                );
+                let join_handle =
+                    run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone());
                 running_tests.insert(id, RunningTest { join_handle });
                 timeout_queue.push_back(TimeoutEntry { id, desc, timeout });
                 pending += 1;
@@ -423,14 +429,7 @@ where
 
             let mut completed_test = res.unwrap();
             let running_test = running_tests.remove(&completed_test.id).unwrap();
-            if let Some(join_handle) = running_test.join_handle {
-                if let Err(_) = join_handle.join() {
-                    if let TrOk = completed_test.result {
-                        completed_test.result =
-                            TrFailedMsg("panicked after reporting success".to_string());
-                    }
-                }
-            }
+            running_test.join(&mut completed_test);
 
             let event = TestEvent::TeResult(completed_test);
             notify_about_test_event(event)?;
@@ -443,8 +442,10 @@ where
         for (id, b) in filtered.benchs {
             let event = TestEvent::TeWait(b.desc.clone());
             notify_about_test_event(event)?;
-            run_test(opts, false, id, b, run_strategy, tx.clone(), Concurrent::No);
-            let completed_test = rx.recv().unwrap();
+            let join_handle = run_test(opts, false, id, b, run_strategy, tx.clone());
+            // Wait for the test to complete.
+            let mut completed_test = rx.recv().unwrap();
+            RunningTest { join_handle }.join(&mut completed_test);
 
             let event = TestEvent::TeResult(completed_test);
             notify_about_test_event(event)?;
@@ -520,7 +521,6 @@ pub fn run_test(
     test: TestDescAndFn,
     strategy: RunStrategy,
     monitor_ch: Sender<CompletedTest>,
-    concurrency: Concurrent,
 ) -> Option<thread::JoinHandle<()>> {
     let TestDescAndFn { desc, testfn } = test;
 
@@ -538,7 +538,6 @@ pub fn run_test(
     struct TestRunOpts {
         pub strategy: RunStrategy,
         pub nocapture: bool,
-        pub concurrency: Concurrent,
         pub time: Option<time::TestTimeOptions>,
     }
 
@@ -549,7 +548,6 @@ pub fn run_test(
         testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
         opts: TestRunOpts,
     ) -> Option<thread::JoinHandle<()>> {
-        let concurrency = opts.concurrency;
         let name = desc.name.clone();
 
         let runtest = move || match opts.strategy {
@@ -576,7 +574,7 @@ pub fn run_test(
         // the test synchronously, regardless of the concurrency
         // level.
         let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_family = "wasm");
-        if concurrency == Concurrent::Yes && supports_threads {
+        if supports_threads {
             let cfg = thread::Builder::new().name(name.as_slice().to_owned());
             let mut runtest = Arc::new(Mutex::new(Some(runtest)));
             let runtest2 = runtest.clone();
@@ -597,7 +595,7 @@ pub fn run_test(
     }
 
     let test_run_opts =
-        TestRunOpts { strategy, nocapture: opts.nocapture, concurrency, time: opts.time_options };
+        TestRunOpts { strategy, nocapture: opts.nocapture, time: opts.time_options };
 
     match testfn {
         DynBenchFn(benchfn) => {

library/test/src/options.rs

@@ -1,12 +1,5 @@
 //! Enums denoting options for test execution.
 
-/// Whether to execute tests concurrently or not
-#[derive(Copy, Clone, Debug, PartialEq, Eq)]
-pub enum Concurrent {
-    Yes,
-    No,
-}
-
 /// Number of times to run a benchmarked function
 #[derive(Clone, PartialEq, Eq)]
 pub enum BenchMode {

library/test/src/tests.rs

@@ -102,7 +102,7 @@ pub fn do_not_run_ignored_tests() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_ne!(result, TrOk);
 }
@@ -125,7 +125,7 @@ pub fn ignored_tests_result_in_ignored() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_eq!(result, TrIgnored);
 }
@@ -150,7 +150,7 @@ fn test_should_panic() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_eq!(result, TrOk);
 }
@@ -175,7 +175,7 @@ fn test_should_panic_good_message() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_eq!(result, TrOk);
 }
@@ -205,7 +205,7 @@ fn test_should_panic_bad_message() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_eq!(result, TrFailedMsg(failed_msg.to_string()));
 }
@@ -239,7 +239,7 @@ fn test_should_panic_non_string_message_type() {
         testfn: DynTestFn(Box::new(f)),
     };
     let (tx, rx) = channel();
-    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
     assert_eq!(result, TrFailedMsg(failed_msg));
 }
@@ -267,15 +267,7 @@ fn test_should_panic_but_succeeds() {
             testfn: DynTestFn(Box::new(f)),
         };
         let (tx, rx) = channel();
-        run_test(
-            &TestOpts::new(),
-            false,
-            TestId(0),
-            desc,
-            RunStrategy::InProcess,
-            tx,
-            Concurrent::No,
-        );
+        run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
         let result = rx.recv().unwrap().result;
         assert_eq!(
             result,
@@ -306,7 +298,7 @@ fn report_time_test_template(report_time: bool) -> Option<TestExecTime> {
 
     let test_opts = TestOpts { time_options, ..TestOpts::new() };
     let (tx, rx) = channel();
-    run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx);
     let exec_time = rx.recv().unwrap().exec_time;
     exec_time
 }
@@ -345,7 +337,7 @@ fn time_test_failure_template(test_type: TestType) -> TestResult {
 
     let test_opts = TestOpts { time_options: Some(time_options), ..TestOpts::new() };
     let (tx, rx) = channel();
-    run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
+    run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx);
     let result = rx.recv().unwrap().result;
 
     result

src/test/run-make-fulldeps/libtest-json/output-default.json

@@ -2,7 +2,7 @@
 { "type": "test", "event": "started", "name": "a" }
 { "type": "test", "name": "a", "event": "ok" }
 { "type": "test", "event": "started", "name": "b" }
-{ "type": "test", "name": "b", "event": "failed", "stdout": "thread 'main' panicked at 'assertion failed: false', f.rs:9:5\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
+{ "type": "test", "name": "b", "event": "failed", "stdout": "thread 'b' panicked at 'assertion failed: false', f.rs:9:5\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
 { "type": "test", "event": "started", "name": "c" }
 { "type": "test", "name": "c", "event": "ok" }
 { "type": "test", "event": "started", "name": "d" }

src/test/run-make-fulldeps/libtest-json/output-stdout-success.json

@@ -2,9 +2,9 @@
 { "type": "test", "event": "started", "name": "a" }
 { "type": "test", "name": "a", "event": "ok", "stdout": "print from successful test\n" }
 { "type": "test", "event": "started", "name": "b" }
-{ "type": "test", "name": "b", "event": "failed", "stdout": "thread 'main' panicked at 'assertion failed: false', f.rs:9:5\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
+{ "type": "test", "name": "b", "event": "failed", "stdout": "thread 'b' panicked at 'assertion failed: false', f.rs:9:5\nnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace\n" }
 { "type": "test", "event": "started", "name": "c" }
-{ "type": "test", "name": "c", "event": "ok", "stdout": "thread 'main' panicked at 'assertion failed: false', f.rs:15:5\n" }
+{ "type": "test", "name": "c", "event": "ok", "stdout": "thread 'c' panicked at 'assertion failed: false', f.rs:15:5\n" }
 { "type": "test", "event": "started", "name": "d" }
 { "type": "test", "name": "d", "event": "ignored", "message": "msg" }
 { "type": "suite", "event": "failed", "passed": 2, "failed": 1, "ignored": 1, "measured": 0, "filtered_out": 0, "exec_time": $TIME }

src/test/ui/test-attrs/test-thread-capture.run.stdout

@@ -10,7 +10,7 @@ fee
 fie
 foe
 fum
-thread 'main' panicked at 'explicit panic', $DIR/test-thread-capture.rs:32:5
+thread 'thready_fail' panicked at 'explicit panic', $DIR/test-thread-capture.rs:32:5
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 
 

src/test/ui/test-attrs/test-thread-nocapture.run.stderr

@@ -1,2 +1,2 @@
-thread 'main' panicked at 'explicit panic', $DIR/test-thread-nocapture.rs:32:5
+thread 'thready_fail' panicked at 'explicit panic', $DIR/test-thread-nocapture.rs:32:5
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace