libtest: Wait for test threads to exit after they report completion

Otherwise we can miss bugs where a test reports that it succeeded but
then panics within a TLS destructor.

Signed-off-by: Anders Kaseorg <[email protected]>

Author: andersk

library/test/src/lib.rs

@@ -25,6 +25,7 @@
 #![feature(nll)]
 #![feature(available_concurrency)]
 #![feature(internal_output_capture)]
+#![feature(option_unwrap_none)]
 #![feature(panic_unwind)]
 #![feature(staged_api)]
 #![feature(termination_trait_lib)]
@@ -208,9 +209,15 @@ where
     use std::collections::{self, HashMap};
     use std::hash::BuildHasherDefault;
     use std::sync::mpsc::RecvTimeoutError;
+
+    struct RunningTest {
+        timeout: Instant,
+        join_handle: Option<thread::JoinHandle<()>>,
+    }
+
     // Use a deterministic hasher
     type TestMap =
-        HashMap<TestDesc, Instant, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
+        HashMap<TestDesc, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
 
     let tests_len = tests.len();
 
@@ -260,7 +267,11 @@ where
         let now = Instant::now();
         let timed_out = running_tests
             .iter()
-            .filter_map(|(desc, timeout)| if &now >= timeout { Some(desc.clone()) } else { None })
+            .filter_map(
+                |(desc, running_test)| {
+                    if now >= running_test.timeout { Some(desc.clone()) } else { None }
+                },
+            )
             .collect();
         for test in &timed_out {
             running_tests.remove(test);
@@ -269,9 +280,9 @@ where
     }
 
     fn calc_timeout(running_tests: &TestMap) -> Option<Duration> {
-        running_tests.values().min().map(|next_timeout| {
+        running_tests.values().map(|running_test| running_test.timeout).min().map(|next_timeout| {
             let now = Instant::now();
-            if *next_timeout >= now { *next_timeout - now } else { Duration::new(0, 0) }
+            if next_timeout >= now { next_timeout - now } else { Duration::new(0, 0) }
         })
     }
 
@@ -280,7 +291,8 @@ where
             let test = remaining.pop().unwrap();
             let event = TestEvent::TeWait(test.desc.clone());
             notify_about_test_event(event)?;
-            run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No);
+            run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No)
+                .unwrap_none();
             let completed_test = rx.recv().unwrap();
 
             let event = TestEvent::TeResult(completed_test);
@@ -291,11 +303,19 @@ where
             while pending < concurrency && !remaining.is_empty() {
                 let test = remaining.pop().unwrap();
                 let timeout = time::get_default_test_timeout();
-                running_tests.insert(test.desc.clone(), timeout);
+                let desc = test.desc.clone();
 
                 let event = TestEvent::TeWait(test.desc.clone());
                 notify_about_test_event(event)?; //here no pad
-                run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::Yes);
+                let join_handle = run_test(
+                    opts,
+                    !opts.run_tests,
+                    test,
+                    run_strategy,
+                    tx.clone(),
+                    Concurrent::Yes,
+                );
+                running_tests.insert(desc, RunningTest { timeout, join_handle });
                 pending += 1;
             }
 
@@ -323,8 +343,16 @@ where
                 }
             }
 
-            let completed_test = res.unwrap();
-            running_tests.remove(&completed_test.desc);
+            let mut completed_test = res.unwrap();
+            let running_test = running_tests.remove(&completed_test.desc).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());
+                    }
+                }
+            }
 
             let event = TestEvent::TeResult(completed_test);
             notify_about_test_event(event)?;
@@ -415,7 +443,7 @@ pub fn run_test(
     strategy: RunStrategy,
     monitor_ch: Sender<CompletedTest>,
     concurrency: Concurrent,
-) {
+) -> Option<thread::JoinHandle<()>> {
     let TestDescAndFn { desc, testfn } = test;
 
     // Emscripten can catch panics but other wasm targets cannot
@@ -426,7 +454,7 @@ pub fn run_test(
     if force_ignore || desc.ignore || ignore_because_no_process_support {
         let message = CompletedTest::new(desc, TrIgnored, None, Vec::new());
         monitor_ch.send(message).unwrap();
-        return;
+        return None;
     }
 
     struct TestRunOpts {
@@ -441,7 +469,7 @@ pub fn run_test(
         monitor_ch: Sender<CompletedTest>,
         testfn: Box<dyn FnOnce() + Send>,
         opts: TestRunOpts,
-    ) {
+    ) -> Option<thread::JoinHandle<()>> {
         let concurrency = opts.concurrency;
         let name = desc.name.clone();
 
@@ -469,9 +497,10 @@ pub fn run_test(
         let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_arch = "wasm32");
         if concurrency == Concurrent::Yes && supports_threads {
             let cfg = thread::Builder::new().name(name.as_slice().to_owned());
-            cfg.spawn(runtest).unwrap();
+            Some(cfg.spawn(runtest).unwrap())
         } else {
             runtest();
+            None
         }
     }
 
@@ -484,10 +513,12 @@ pub fn run_test(
             crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| {
                 bencher.run(harness)
             });
+            None
         }
         StaticBenchFn(benchfn) => {
             // Benchmarks aren't expected to panic, so we run them all in-process.
             crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn);
+            None
         }
         DynTestFn(f) => {
             match strategy {
@@ -499,7 +530,7 @@ pub fn run_test(
                 monitor_ch,
                 Box::new(move || __rust_begin_short_backtrace(f)),
                 test_run_opts,
-            );
+            )
         }
         StaticTestFn(f) => run_test_inner(
             desc,