Auto merge of #111992 - ferrocene:pa-panic-abort-tests-bench, r=m-ou-se

Test benchmarks with `-Z panic-abort-tests`

During test execution, when a `#[bench]` benchmark is encountered it's executed once to check whether it works. Unfortunately that was not compatible with `-Z panic-abort-tests`: the feature works by spawning a subprocess for each test, which prevents the use of dynamic tests as we cannot pass closures to child processes, and before this PR the conversion from benchmark to test was done by turning benchmarks into dynamic tests whose closures execute the benchmark once.

The approach this PR took was to add two new kinds of `TestFn`s: `StaticBenchAsTestFn` and `DynBenchAsTestFn` (:warning: **this is a breaking change** :warning:). With that change, a `StaticBenchFn` can be converted into a `StaticBenchAsTestFn` without creating dynamic tests, and making it possible to test `#[bench]` functions with `-Z panic-abort-tests`. The subprocess test runner also had to be updated to perform the conversion from benchmark to test when appropriate.

Along with the bug fix, in the first commit I refactored how tests are executed: rather than executing the test function in multiple places across `libtest`, there is now a private `TestFn::into_runnable()` method, which returns either a `RunnableTest` or `RunnableBench`, on which you can call the `run()` method. This simplified the rest of the changes in the PR.

This PR is best reviewed commit-by-commit.
Fixes https://github.com/rust-lang/rust/issues/73509

6 files changed, 184 insertions, 122 deletions

diff --git a/library/test/src/console.rs b/library/test/src/console.rs
index 7eee4ca2361..bbeb944e8b1 100644
--- a/library/test/src/console.rs
+++ b/library/test/src/console.rs
@@ -199,7 +199,7 @@ pub fn list_tests_console(opts: &TestOpts, tests: Vec<TestDescAndFn>) -> io::Res
         let TestDescAndFn { desc, testfn } = test;
 
         let fntype = match testfn {
-            StaticTestFn(..) | DynTestFn(..) => {
+            StaticTestFn(..) | DynTestFn(..) | StaticBenchAsTestFn(..) | DynBenchAsTestFn(..) => {
                 st.tests += 1;
                 "test"
             }
diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs
index e76d6716b94..b40b6009ea4 100644
--- a/library/test/src/lib.rs
+++ b/library/test/src/lib.rs
@@ -92,6 +92,7 @@ use time::TestExecTime;
 const ERROR_EXIT_CODE: i32 = 101;
 
 const SECONDARY_TEST_INVOKER_VAR: &str = "__RUST_TEST_INVOKE";
+const SECONDARY_TEST_BENCH_BENCHMARKS_VAR: &str = "__RUST_TEST_BENCH_BENCHMARKS";
 
 // The default console test runner. It accepts the command line
 // arguments and a vector of test_descs.
@@ -171,18 +172,32 @@ pub fn test_main_static_abort(tests: &[&TestDescAndFn]) {
     // will then exit the process.
     if let Ok(name) = env::var(SECONDARY_TEST_INVOKER_VAR) {
         env::remove_var(SECONDARY_TEST_INVOKER_VAR);
+
+        // Convert benchmarks to tests if we're not benchmarking.
+        let mut tests = tests.iter().map(make_owned_test).collect::<Vec<_>>();
+        if env::var(SECONDARY_TEST_BENCH_BENCHMARKS_VAR).is_ok() {
+            env::remove_var(SECONDARY_TEST_BENCH_BENCHMARKS_VAR);
+        } else {
+            tests = convert_benchmarks_to_tests(tests);
+        };
+
         let test = tests
-            .iter()
+            .into_iter()
             .filter(|test| test.desc.name.as_slice() == name)
-            .map(make_owned_test)
             .next()
             .unwrap_or_else(|| panic!("couldn't find a test with the provided name '{name}'"));
         let TestDescAndFn { desc, testfn } = test;
-        let testfn = match testfn {
-            StaticTestFn(f) => f,
-            _ => panic!("only static tests are supported"),
-        };
-        run_test_in_spawned_subprocess(desc, Box::new(testfn));
+        match testfn.into_runnable() {
+            Runnable::Test(runnable_test) => {
+                if runnable_test.is_dynamic() {
+                    panic!("only static tests are supported");
+                }
+                run_test_in_spawned_subprocess(desc, runnable_test);
+            }
+            Runnable::Bench(_) => {
+                panic!("benchmarks should not be executed into child processes")
+            }
+        }
     }
 
     let args = env::args().collect::<Vec<_>>();
@@ -234,16 +249,6 @@ impl FilteredTests {
         self.tests.push((TestId(self.next_id), test));
         self.next_id += 1;
     }
-    fn add_bench_as_test(
-        &mut self,
-        desc: TestDesc,
-        benchfn: impl Fn(&mut Bencher) -> Result<(), String> + Send + 'static,
-    ) {
-        let testfn = DynTestFn(Box::new(move || {
-            bench::run_once(|b| __rust_begin_short_backtrace(|| benchfn(b)))
-        }));
-        self.add_test(desc, testfn);
-    }
     fn total_len(&self) -> usize {
         self.tests.len() + self.benches.len()
     }
@@ -301,14 +306,14 @@ where
                 if opts.bench_benchmarks {
                     filtered.add_bench(desc, DynBenchFn(benchfn));
                 } else {
-                    filtered.add_bench_as_test(desc, benchfn);
+                    filtered.add_test(desc, DynBenchAsTestFn(benchfn));
                 }
             }
             StaticBenchFn(benchfn) => {
                 if opts.bench_benchmarks {
                     filtered.add_bench(desc, StaticBenchFn(benchfn));
                 } else {
-                    filtered.add_bench_as_test(desc, benchfn);
+                    filtered.add_test(desc, StaticBenchAsTestFn(benchfn));
                 }
             }
             testfn => {
@@ -519,12 +524,8 @@ pub fn convert_benchmarks_to_tests(tests: Vec<TestDescAndFn>) -> Vec<TestDescAnd
         .into_iter()
         .map(|x| {
             let testfn = match x.testfn {
-                DynBenchFn(benchfn) => DynTestFn(Box::new(move || {
-                    bench::run_once(|b| __rust_begin_short_backtrace(|| benchfn(b)))
-                })),
-                StaticBenchFn(benchfn) => DynTestFn(Box::new(move || {
-                    bench::run_once(|b| __rust_begin_short_backtrace(|| benchfn(b)))
-                })),
+                DynBenchFn(benchfn) => DynBenchAsTestFn(benchfn),
+                StaticBenchFn(benchfn) => StaticBenchAsTestFn(benchfn),
                 f => f,
             };
             TestDescAndFn { desc: x.desc, testfn }
@@ -553,99 +554,69 @@ pub fn run_test(
         return None;
     }
 
-    struct TestRunOpts {
-        pub strategy: RunStrategy,
-        pub nocapture: bool,
-        pub time: Option<time::TestTimeOptions>,
-    }
+    match testfn.into_runnable() {
+        Runnable::Test(runnable_test) => {
+            if runnable_test.is_dynamic() {
+                match strategy {
+                    RunStrategy::InProcess => (),
+                    _ => panic!("Cannot run dynamic test fn out-of-process"),
+                };
+            }
 
-    fn run_test_inner(
-        id: TestId,
-        desc: TestDesc,
-        monitor_ch: Sender<CompletedTest>,
-        testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
-        opts: TestRunOpts,
-    ) -> Option<thread::JoinHandle<()>> {
-        let name = desc.name.clone();
-
-        let runtest = move || match opts.strategy {
-            RunStrategy::InProcess => run_test_in_process(
-                id,
-                desc,
-                opts.nocapture,
-                opts.time.is_some(),
-                testfn,
-                monitor_ch,
-                opts.time,
-            ),
-            RunStrategy::SpawnPrimary => spawn_test_subprocess(
-                id,
-                desc,
-                opts.nocapture,
-                opts.time.is_some(),
-                monitor_ch,
-                opts.time,
-            ),
-        };
+            let name = desc.name.clone();
+            let nocapture = opts.nocapture;
+            let time_options = opts.time_options;
+            let bench_benchmarks = opts.bench_benchmarks;
+
+            let runtest = move || match strategy {
+                RunStrategy::InProcess => run_test_in_process(
+                    id,
+                    desc,
+                    nocapture,
+                    time_options.is_some(),
+                    runnable_test,
+                    monitor_ch,
+                    time_options,
+                ),
+                RunStrategy::SpawnPrimary => spawn_test_subprocess(
+                    id,
+                    desc,
+                    nocapture,
+                    time_options.is_some(),
+                    monitor_ch,
+                    time_options,
+                    bench_benchmarks,
+                ),
+            };
 
-        // If the platform is single-threaded we're just going to run
-        // the test synchronously, regardless of the concurrency
-        // level.
-        let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_family = "wasm");
-        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();
-            match cfg.spawn(move || runtest2.lock().unwrap().take().unwrap()()) {
-                Ok(handle) => Some(handle),
-                Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
-                    // `ErrorKind::WouldBlock` means hitting the thread limit on some
-                    // platforms, so run the test synchronously here instead.
-                    Arc::get_mut(&mut runtest).unwrap().get_mut().unwrap().take().unwrap()();
-                    None
+            // If the platform is single-threaded we're just going to run
+            // the test synchronously, regardless of the concurrency
+            // level.
+            let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_family = "wasm");
+            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();
+                match cfg.spawn(move || runtest2.lock().unwrap().take().unwrap()()) {
+                    Ok(handle) => Some(handle),
+                    Err(e) if e.kind() == io::ErrorKind::WouldBlock => {
+                        // `ErrorKind::WouldBlock` means hitting the thread limit on some
+                        // platforms, so run the test synchronously here instead.
+                        Arc::get_mut(&mut runtest).unwrap().get_mut().unwrap().take().unwrap()();
+                        None
+                    }
+                    Err(e) => panic!("failed to spawn thread to run test: {e}"),
                 }
-                Err(e) => panic!("failed to spawn thread to run test: {e}"),
+            } else {
+                runtest();
+                None
             }
-        } else {
-            runtest();
-            None
         }
-    }
-
-    let test_run_opts =
-        TestRunOpts { strategy, nocapture: opts.nocapture, time: opts.time_options };
-
-    match testfn {
-        DynBenchFn(benchfn) => {
+        Runnable::Bench(runnable_bench) => {
             // Benchmarks aren't expected to panic, so we run them all in-process.
-            crate::bench::benchmark(id, desc, monitor_ch, opts.nocapture, benchfn);
+            runnable_bench.run(id, &desc, &monitor_ch, opts.nocapture);
             None
         }
-        StaticBenchFn(benchfn) => {
-            // Benchmarks aren't expected to panic, so we run them all in-process.
-            crate::bench::benchmark(id, desc, monitor_ch, opts.nocapture, benchfn);
-            None
-        }
-        DynTestFn(f) => {
-            match strategy {
-                RunStrategy::InProcess => (),
-                _ => panic!("Cannot run dynamic test fn out-of-process"),
-            };
-            run_test_inner(
-                id,
-                desc,
-                monitor_ch,
-                Box::new(move || __rust_begin_short_backtrace(f)),
-                test_run_opts,
-            )
-        }
-        StaticTestFn(f) => run_test_inner(
-            id,
-            desc,
-            monitor_ch,
-            Box::new(move || __rust_begin_short_backtrace(f)),
-            test_run_opts,
-        ),
     }
 }
 
@@ -663,7 +634,7 @@ fn run_test_in_process(
     desc: TestDesc,
     nocapture: bool,
     report_time: bool,
-    testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
+    runnable_test: RunnableTest,
     monitor_ch: Sender<CompletedTest>,
     time_opts: Option<time::TestTimeOptions>,
 ) {
@@ -675,7 +646,7 @@ fn run_test_in_process(
     }
 
     let start = report_time.then(Instant::now);
-    let result = fold_err(catch_unwind(AssertUnwindSafe(testfn)));
+    let result = fold_err(catch_unwind(AssertUnwindSafe(|| runnable_test.run())));
     let exec_time = start.map(|start| {
         let duration = start.elapsed();
         TestExecTime(duration)
@@ -712,6 +683,7 @@ fn spawn_test_subprocess(
     report_time: bool,
     monitor_ch: Sender<CompletedTest>,
     time_opts: Option<time::TestTimeOptions>,
+    bench_benchmarks: bool,
 ) {
     let (result, test_output, exec_time) = (|| {
         let args = env::args().collect::<Vec<_>>();
@@ -719,6 +691,9 @@ fn spawn_test_subprocess(
 
         let mut command = Command::new(current_exe);
         command.env(SECONDARY_TEST_INVOKER_VAR, desc.name.as_slice());
+        if bench_benchmarks {
+            command.env(SECONDARY_TEST_BENCH_BENCHMARKS_VAR, "1");
+        }
         if nocapture {
             command.stdout(process::Stdio::inherit());
             command.stderr(process::Stdio::inherit());
@@ -760,10 +735,7 @@ fn spawn_test_subprocess(
     monitor_ch.send(message).unwrap();
 }
 
-fn run_test_in_spawned_subprocess(
-    desc: TestDesc,
-    testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
-) -> ! {
+fn run_test_in_spawned_subprocess(desc: TestDesc, runnable_test: RunnableTest) -> ! {
     let builtin_panic_hook = panic::take_hook();
     let record_result = Arc::new(move |panic_info: Option<&'_ PanicInfo<'_>>| {
         let test_result = match panic_info {
@@ -789,7 +761,7 @@ fn run_test_in_spawned_subprocess(
     });
     let record_result2 = record_result.clone();
     panic::set_hook(Box::new(move |info| record_result2(Some(info))));
-    if let Err(message) = testfn() {
+    if let Err(message) = runnable_test.run() {
         panic!("{}", message);
     }
     record_result(None);
diff --git a/library/test/src/types.rs b/library/test/src/types.rs
index e79914dbf4b..504ceee7fce 100644
--- a/library/test/src/types.rs
+++ b/library/test/src/types.rs
@@ -2,8 +2,11 @@
 
 use std::borrow::Cow;
 use std::fmt;
+use std::sync::mpsc::Sender;
 
+use super::__rust_begin_short_backtrace;
 use super::bench::Bencher;
+use super::event::CompletedTest;
 use super::options;
 
 pub use NamePadding::*;
@@ -82,8 +85,10 @@ impl fmt::Display for TestName {
 pub enum TestFn {
     StaticTestFn(fn() -> Result<(), String>),
     StaticBenchFn(fn(&mut Bencher) -> Result<(), String>),
+    StaticBenchAsTestFn(fn(&mut Bencher) -> Result<(), String>),
     DynTestFn(Box<dyn FnOnce() -> Result<(), String> + Send>),
     DynBenchFn(Box<dyn Fn(&mut Bencher) -> Result<(), String> + Send>),
+    DynBenchAsTestFn(Box<dyn Fn(&mut Bencher) -> Result<(), String> + Send>),
 }
 
 impl TestFn {
@@ -91,8 +96,21 @@ impl TestFn {
         match *self {
             StaticTestFn(..) => PadNone,
             StaticBenchFn(..) => PadOnRight,
+            StaticBenchAsTestFn(..) => PadNone,
             DynTestFn(..) => PadNone,
             DynBenchFn(..) => PadOnRight,
+            DynBenchAsTestFn(..) => PadNone,
+        }
+    }
+
+    pub(crate) fn into_runnable(self) -> Runnable {
+        match self {
+            StaticTestFn(f) => Runnable::Test(RunnableTest::Static(f)),
+            StaticBenchFn(f) => Runnable::Bench(RunnableBench::Static(f)),
+            StaticBenchAsTestFn(f) => Runnable::Test(RunnableTest::StaticBenchAsTest(f)),
+            DynTestFn(f) => Runnable::Test(RunnableTest::Dynamic(f)),
+            DynBenchFn(f) => Runnable::Bench(RunnableBench::Dynamic(f)),
+            DynBenchAsTestFn(f) => Runnable::Test(RunnableTest::DynamicBenchAsTest(f)),
         }
     }
 }
@@ -102,12 +120,74 @@ impl fmt::Debug for TestFn {
         f.write_str(match *self {
             StaticTestFn(..) => "StaticTestFn(..)",
             StaticBenchFn(..) => "StaticBenchFn(..)",
+            StaticBenchAsTestFn(..) => "StaticBenchAsTestFn(..)",
             DynTestFn(..) => "DynTestFn(..)",
             DynBenchFn(..) => "DynBenchFn(..)",
+            DynBenchAsTestFn(..) => "DynBenchAsTestFn(..)",
         })
     }
 }
 
+pub(crate) enum Runnable {
+    Test(RunnableTest),
+    Bench(RunnableBench),
+}
+
+pub(crate) enum RunnableTest {
+    Static(fn() -> Result<(), String>),
+    Dynamic(Box<dyn FnOnce() -> Result<(), String> + Send>),
+    StaticBenchAsTest(fn(&mut Bencher) -> Result<(), String>),
+    DynamicBenchAsTest(Box<dyn Fn(&mut Bencher) -> Result<(), String> + Send>),
+}
+
+impl RunnableTest {
+    pub(crate) fn run(self) -> Result<(), String> {
+        match self {
+            RunnableTest::Static(f) => __rust_begin_short_backtrace(f),
+            RunnableTest::Dynamic(f) => __rust_begin_short_backtrace(f),
+            RunnableTest::StaticBenchAsTest(f) => {
+                crate::bench::run_once(|b| __rust_begin_short_backtrace(|| f(b)))
+            }
+            RunnableTest::DynamicBenchAsTest(f) => {
+                crate::bench::run_once(|b| __rust_begin_short_backtrace(|| f(b)))
+            }
+        }
+    }
+
+    pub(crate) fn is_dynamic(&self) -> bool {
+        match self {
+            RunnableTest::Static(_) => false,
+            RunnableTest::StaticBenchAsTest(_) => false,
+            RunnableTest::Dynamic(_) => true,
+            RunnableTest::DynamicBenchAsTest(_) => true,
+        }
+    }
+}
+
+pub(crate) enum RunnableBench {
+    Static(fn(&mut Bencher) -> Result<(), String>),
+    Dynamic(Box<dyn Fn(&mut Bencher) -> Result<(), String> + Send>),
+}
+
+impl RunnableBench {
+    pub(crate) fn run(
+        self,
+        id: TestId,
+        desc: &TestDesc,
+        monitor_ch: &Sender<CompletedTest>,
+        nocapture: bool,
+    ) {
+        match self {
+            RunnableBench::Static(f) => {
+                crate::bench::benchmark(id, desc.clone(), monitor_ch.clone(), nocapture, f)
+            }
+            RunnableBench::Dynamic(f) => {
+                crate::bench::benchmark(id, desc.clone(), monitor_ch.clone(), nocapture, f)
+            }
+        }
+    }
+}
+
 // A unique integer associated with each test.
 #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
 pub struct TestId(pub usize);
diff --git a/tests/ui/test-attrs/test-fn-signature-verification-for-explicit-return-type.rs b/tests/ui/test-attrs/test-fn-signature-verification-for-explicit-return-type.rs
index 585874e273d..02fee1a00da 100644
--- a/tests/ui/test-attrs/test-fn-signature-verification-for-explicit-return-type.rs
+++ b/tests/ui/test-attrs/test-fn-signature-verification-for-explicit-return-type.rs
@@ -1,5 +1,5 @@
 // run-pass
-// needs-unwind (#73509)
+// ignore-fuchsia Test must be run out-of-process
 
 #![feature(test)]
 
diff --git a/tests/ui/test-attrs/test-panic-abort.rs b/tests/ui/test-attrs/test-panic-abort.rs
index 931b7993c81..08e5242af66 100644
--- a/tests/ui/test-attrs/test-panic-abort.rs
+++ b/tests/ui/test-attrs/test-panic-abort.rs
@@ -11,9 +11,13 @@
 // ignore-sgx no subprocess support
 
 #![cfg(test)]
+#![feature(test)]
+
+extern crate test;
 
 use std::io::Write;
 use std::env;
+use test::Bencher;
 
 #[test]
 fn it_works() {
@@ -48,3 +52,8 @@ fn no_residual_environment() {
         }
     }
 }
+
+#[bench]
+fn benchmark(b: &mut Bencher) {
+    b.iter(|| assert_eq!(1 + 1, 2));
+}
diff --git a/tests/ui/test-attrs/test-panic-abort.run.stdout b/tests/ui/test-attrs/test-panic-abort.run.stdout
index f608a8cdc55..b6b9c2560fe 100644
--- a/tests/ui/test-attrs/test-panic-abort.run.stdout
+++ b/tests/ui/test-attrs/test-panic-abort.run.stdout
@@ -1,5 +1,6 @@
 
-running 5 tests
+running 6 tests
+test benchmark ... ok
 test it_exits ... FAILED
 test it_fails ... FAILED
 test it_panics - should panic ... ok
@@ -18,7 +19,7 @@ testing123
 testing321
 thread 'main' panicked at 'assertion failed: `(left == right)`
   left: `2`,
- right: `5`', $DIR/test-panic-abort.rs:34:5
+ right: `5`', $DIR/test-panic-abort.rs:38:5
 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 
 
@@ -26,5 +27,5 @@ failures:
     it_exits
     it_fails
 
-test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME
+test result: FAILED. 4 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in $TIME