From a47eabe454bbab872019f4ce19960991a5338ece Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 14:49:38 +0200 Subject: [PATCH 1/4] refactor executing tests to centralize actually invoking tests Before this commit, tests were invoked in multiple places, especially due to `-Z panic-abort-tests`, and adding a new test kind meant having to chase down and update all these places. This commit creates a new Runnable enum, and its children RunnableTest and RunnableBench. The rest of the harness will now pass around the enum rather than constructing and passing around boxed functions. The enum has two children enums because invoking tests and invoking benchmarks requires different parameters. --- test/src/lib.rs | 69 +++++++++++++++++++---------------------------- test/src/types.rs | 62 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 41 deletions(-) diff --git a/test/src/lib.rs b/test/src/lib.rs index e76d6716b..ca8000e99 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -178,11 +178,17 @@ pub fn test_main_static_abort(tests: &[&TestDescAndFn]) { .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::>(); @@ -563,7 +569,7 @@ pub fn run_test( id: TestId, desc: TestDesc, monitor_ch: Sender, - testfn: Box Result<(), String> + Send>, + runnable_test: RunnableTest, opts: TestRunOpts, ) -> Option> { let name = desc.name.clone(); @@ -574,7 +580,7 @@ pub fn run_test( desc, opts.nocapture, opts.time.is_some(), - testfn, + runnable_test, monitor_ch, opts.time, ), @@ -615,37 +621,21 @@ pub fn run_test( let test_run_opts = TestRunOpts { strategy, nocapture: opts.nocapture, time: opts.time_options }; - match testfn { - DynBenchFn(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 + 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"), + }; + } + run_test_inner(id, desc, monitor_ch, runnable_test, test_run_opts) } - StaticBenchFn(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 } - 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 +653,7 @@ fn run_test_in_process( desc: TestDesc, nocapture: bool, report_time: bool, - testfn: Box Result<(), String> + Send>, + runnable_test: RunnableTest, monitor_ch: Sender, time_opts: Option, ) { @@ -675,7 +665,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) @@ -760,10 +750,7 @@ fn spawn_test_subprocess( monitor_ch.send(message).unwrap(); } -fn run_test_in_spawned_subprocess( - desc: TestDesc, - testfn: Box 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 +776,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/test/src/types.rs b/test/src/types.rs index e79914dbf..b67880a98 100644 --- a/test/src/types.rs +++ b/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::*; @@ -95,6 +98,15 @@ impl TestFn { DynBenchFn(..) => PadOnRight, } } + + pub(crate) fn into_runnable(self) -> Runnable { + match self { + StaticTestFn(f) => Runnable::Test(RunnableTest::Static(f)), + StaticBenchFn(f) => Runnable::Bench(RunnableBench::Static(f)), + DynTestFn(f) => Runnable::Test(RunnableTest::Dynamic(f)), + DynBenchFn(f) => Runnable::Bench(RunnableBench::Dynamic(f)), + } + } } impl fmt::Debug for TestFn { @@ -108,6 +120,56 @@ impl fmt::Debug for TestFn { } } +pub(crate) enum Runnable { + Test(RunnableTest), + Bench(RunnableBench), +} + +pub(crate) enum RunnableTest { + Static(fn() -> Result<(), String>), + Dynamic(Box 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), + } + } + + pub(crate) fn is_dynamic(&self) -> bool { + match self { + RunnableTest::Static(_) => false, + RunnableTest::Dynamic(_) => true, + } + } +} + +pub(crate) enum RunnableBench { + Static(fn(&mut Bencher) -> Result<(), String>), + Dynamic(Box Result<(), String> + Send>), +} + +impl RunnableBench { + pub(crate) fn run( + self, + id: TestId, + desc: &TestDesc, + monitor_ch: &Sender, + 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); From 03a11644e01201df33b58a1f27cd82b8977efe45 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 14:56:59 +0200 Subject: [PATCH 2/4] remove nested function from run_test The inner function is not needed anymore as it's only called once after the previous commit's refactoring. --- test/src/lib.rs | 110 +++++++++++++++++++++--------------------------- 1 file changed, 47 insertions(+), 63 deletions(-) diff --git a/test/src/lib.rs b/test/src/lib.rs index ca8000e99..c2725a432 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -559,68 +559,6 @@ pub fn run_test( return None; } - struct TestRunOpts { - pub strategy: RunStrategy, - pub nocapture: bool, - pub time: Option, - } - - fn run_test_inner( - id: TestId, - desc: TestDesc, - monitor_ch: Sender, - runnable_test: RunnableTest, - opts: TestRunOpts, - ) -> Option> { - 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(), - runnable_test, - monitor_ch, - opts.time, - ), - RunStrategy::SpawnPrimary => spawn_test_subprocess( - id, - desc, - opts.nocapture, - opts.time.is_some(), - monitor_ch, - opts.time, - ), - }; - - // 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}"), - } - } else { - runtest(); - None - } - } - - let test_run_opts = - TestRunOpts { strategy, nocapture: opts.nocapture, time: opts.time_options }; - match testfn.into_runnable() { Runnable::Test(runnable_test) => { if runnable_test.is_dynamic() { @@ -629,7 +567,53 @@ pub fn run_test( _ => panic!("Cannot run dynamic test fn out-of-process"), }; } - run_test_inner(id, desc, monitor_ch, runnable_test, test_run_opts) + + let name = desc.name.clone(); + let nocapture = opts.nocapture; + let time_options = opts.time_options; + + 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, + ), + }; + + // 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}"), + } + } else { + runtest(); + None + } } Runnable::Bench(runnable_bench) => { // Benchmarks aren't expected to panic, so we run them all in-process. From 231ce1365bf5f0751258df600f5255067245c53f Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 12:46:54 +0200 Subject: [PATCH 3/4] add StaticBenchAsTestFn and DynBenchAsTestFn to convert benches to tests Before this commit, both static and dynamic benches were converted to a DynTestFn, with a boxed closure that ran the benchmarks exactly once. While this worked, it conflicted with -Z panic-abort-tests as the flag does not support dynamic tests. With this change, a StaticBenchFn is converted to a StaticBenchAsTestFn, avoiding any dynamic test creation. DynBenchFn is also converted to DynBenchAsTestFn for completeness. --- test/src/console.rs | 2 +- test/src/lib.rs | 22 ++++------------------ test/src/types.rs | 18 ++++++++++++++++++ 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/test/src/console.rs b/test/src/console.rs index 7eee4ca23..bbeb944e8 100644 --- a/test/src/console.rs +++ b/test/src/console.rs @@ -199,7 +199,7 @@ pub fn list_tests_console(opts: &TestOpts, tests: Vec) -> io::Res let TestDescAndFn { desc, testfn } = test; let fntype = match testfn { - StaticTestFn(..) | DynTestFn(..) => { + StaticTestFn(..) | DynTestFn(..) | StaticBenchAsTestFn(..) | DynBenchAsTestFn(..) => { st.tests += 1; "test" } diff --git a/test/src/lib.rs b/test/src/lib.rs index c2725a432..786439e0a 100644 --- a/test/src/lib.rs +++ b/test/src/lib.rs @@ -240,16 +240,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() } @@ -307,14 +297,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 => { @@ -525,12 +515,8 @@ pub fn convert_benchmarks_to_tests(tests: Vec) -> Vec 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 } diff --git a/test/src/types.rs b/test/src/types.rs index b67880a98..504ceee7f 100644 --- a/test/src/types.rs +++ b/test/src/types.rs @@ -85,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 Result<(), String> + Send>), DynBenchFn(Box Result<(), String> + Send>), + DynBenchAsTestFn(Box Result<(), String> + Send>), } impl TestFn { @@ -94,8 +96,10 @@ impl TestFn { match *self { StaticTestFn(..) => PadNone, StaticBenchFn(..) => PadOnRight, + StaticBenchAsTestFn(..) => PadNone, DynTestFn(..) => PadNone, DynBenchFn(..) => PadOnRight, + DynBenchAsTestFn(..) => PadNone, } } @@ -103,8 +107,10 @@ impl TestFn { 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)), } } } @@ -114,8 +120,10 @@ impl fmt::Debug for TestFn { f.write_str(match *self { StaticTestFn(..) => "StaticTestFn(..)", StaticBenchFn(..) => "StaticBenchFn(..)", + StaticBenchAsTestFn(..) => "StaticBenchAsTestFn(..)", DynTestFn(..) => "DynTestFn(..)", DynBenchFn(..) => "DynBenchFn(..)", + DynBenchAsTestFn(..) => "DynBenchAsTestFn(..)", }) } } @@ -128,6 +136,8 @@ pub(crate) enum Runnable { pub(crate) enum RunnableTest { Static(fn() -> Result<(), String>), Dynamic(Box Result<(), String> + Send>), + StaticBenchAsTest(fn(&mut Bencher) -> Result<(), String>), + DynamicBenchAsTest(Box Result<(), String> + Send>), } impl RunnableTest { @@ -135,13 +145,21 @@ impl RunnableTest { 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, } } } From 88db75429e5093e2b8541827b5a5d8c728c80e86 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 12:56:16 +0200 Subject: [PATCH 4/4] convert benches to tests in subprocess if we're not benchmarking --- test/src/lib.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/test/src/lib.rs b/test/src/lib.rs index 786439e0a..b40b6009e 100644 --- a/test/src/lib.rs +++ b/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,10 +172,18 @@ 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::>(); + 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; @@ -557,6 +566,7 @@ pub fn run_test( 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( @@ -575,6 +585,7 @@ pub fn run_test( time_options.is_some(), monitor_ch, time_options, + bench_benchmarks, ), }; @@ -672,6 +683,7 @@ fn spawn_test_subprocess( report_time: bool, monitor_ch: Sender, time_opts: Option, + bench_benchmarks: bool, ) { let (result, test_output, exec_time) = (|| { let args = env::args().collect::>(); @@ -679,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());