Skip to content

Commit

Permalink
Auto merge of #111992 - ferrocene:pa-panic-abort-tests-bench, r=m-ou-se
Browse files Browse the repository at this point in the history 
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 #73509
  • Loading branch information
@bors
bors committed Jul 1, 2023
2 parents 6b06fdf + 58d9d8c commit e5bb341
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 122 deletions.
2 changes: 1 addition & 1 deletion library/test/src/console.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down
206 changes: 89 additions & 117 deletions library/test/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<_>>();
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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 => {
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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,
),
}
}

Expand All @@ -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>,
) {
Expand All @@ -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)
Expand Down Expand Up @@ -712,13 +683,17 @@ 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<_>>();
let current_exe = &args[0];

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());
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
Expand Down
Loading

0 comments on commit e5bb341

Please sign in to comment.