From 3c42d9fe20aff3c720d1f390573450d336cca3f8 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Fri, 19 Feb 2021 10:33:03 -0800 Subject: [PATCH] libtest: Index tests by a unique TestId This more robustly avoids problems with duplicate TestDesc. See #81852 and #82274. Signed-off-by: Anders Kaseorg --- library/test/src/bench.rs | 17 +++++++++--- library/test/src/event.rs | 6 +++-- library/test/src/lib.rs | 57 +++++++++++++++++++++++---------------- library/test/src/tests.rs | 30 +++++++++++++-------- library/test/src/types.rs | 6 ++++- 5 files changed, 75 insertions(+), 41 deletions(-) diff --git a/library/test/src/bench.rs b/library/test/src/bench.rs index d4b37284ea774..169154187f250 100644 --- a/library/test/src/bench.rs +++ b/library/test/src/bench.rs @@ -2,7 +2,11 @@ pub use std::hint::black_box; use super::{ - event::CompletedTest, options::BenchMode, test_result::TestResult, types::TestDesc, Sender, + event::CompletedTest, + options::BenchMode, + test_result::TestResult, + types::{TestDesc, TestId}, + Sender, }; use crate::stats; @@ -177,8 +181,13 @@ where } } -pub fn benchmark(desc: TestDesc, monitor_ch: Sender, nocapture: bool, f: F) -where +pub fn benchmark( + id: TestId, + desc: TestDesc, + monitor_ch: Sender, + nocapture: bool, + f: F, +) where F: FnMut(&mut Bencher), { let mut bs = Bencher { mode: BenchMode::Auto, summary: None, bytes: 0 }; @@ -213,7 +222,7 @@ where }; let stdout = data.lock().unwrap().to_vec(); - let message = CompletedTest::new(desc, test_result, None, stdout); + let message = CompletedTest::new(id, desc, test_result, None, stdout); monitor_ch.send(message).unwrap(); } diff --git a/library/test/src/event.rs b/library/test/src/event.rs index 2103a0d10f4c6..206f3e10e847d 100644 --- a/library/test/src/event.rs +++ b/library/test/src/event.rs @@ -3,10 +3,11 @@ use super::test_result::TestResult; use super::time::TestExecTime; -use super::types::TestDesc; +use super::types::{TestDesc, TestId}; #[derive(Debug, Clone)] pub struct CompletedTest { + pub id: TestId, pub desc: TestDesc, pub result: TestResult, pub exec_time: Option, @@ -15,12 +16,13 @@ pub struct CompletedTest { impl CompletedTest { pub fn new( + id: TestId, desc: TestDesc, result: TestResult, exec_time: Option, stdout: Vec, ) -> Self { - Self { desc, result, exec_time, stdout } + Self { id, desc, result, exec_time, stdout } } } diff --git a/library/test/src/lib.rs b/library/test/src/lib.rs index 7683f792b8dbf..2e0864f303cc9 100644 --- a/library/test/src/lib.rs +++ b/library/test/src/lib.rs @@ -54,7 +54,7 @@ pub mod test { time::{TestExecTime, TestTimeOptions}, types::{ DynTestFn, DynTestName, StaticBenchFn, StaticTestFn, StaticTestName, TestDesc, - TestDescAndFn, TestName, TestType, + TestDescAndFn, TestId, TestName, TestType, }, }; } @@ -215,9 +215,10 @@ where // Use a deterministic hasher type TestMap = - HashMap>; + HashMap>; struct TimeoutEntry { + id: TestId, desc: TestDesc, timeout: Instant, } @@ -249,7 +250,9 @@ where let (filtered_tests, filtered_benchs): (Vec<_>, _) = filtered_tests .into_iter() - .partition(|e| matches!(e.testfn, StaticTestFn(_) | DynTestFn(_))); + .enumerate() + .map(|(i, e)| (TestId(i), e)) + .partition(|(_, e)| matches!(e.testfn, StaticTestFn(_) | DynTestFn(_))); let concurrency = opts.test_threads.unwrap_or_else(get_concurrency); @@ -278,7 +281,7 @@ where break; } let timeout_entry = timeout_queue.pop_front().unwrap(); - if running_tests.contains_key(&timeout_entry.desc) { + if running_tests.contains_key(&timeout_entry.id) { timed_out.push(timeout_entry.desc); } } @@ -294,11 +297,11 @@ where if concurrency == 1 { while !remaining.is_empty() { - let test = remaining.pop().unwrap(); + let (id, test) = remaining.pop().unwrap(); let event = TestEvent::TeWait(test.desc.clone()); notify_about_test_event(event)?; let join_handle = - run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No); + 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(); @@ -308,7 +311,7 @@ where } else { while pending > 0 || !remaining.is_empty() { while pending < concurrency && !remaining.is_empty() { - let test = remaining.pop().unwrap(); + let (id, test) = remaining.pop().unwrap(); let timeout = time::get_default_test_timeout(); let desc = test.desc.clone(); @@ -317,13 +320,14 @@ where let join_handle = run_test( opts, !opts.run_tests, + id, test, run_strategy, tx.clone(), Concurrent::Yes, ); - running_tests.insert(desc.clone(), RunningTest { join_handle }); - timeout_queue.push_back(TimeoutEntry { desc, timeout }); + running_tests.insert(id, RunningTest { join_handle }); + timeout_queue.push_back(TimeoutEntry { id, desc, timeout }); pending += 1; } @@ -352,13 +356,12 @@ where } let mut completed_test = res.unwrap(); - if let Some(running_test) = running_tests.remove(&completed_test.desc) { - 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 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()); } } } @@ -371,10 +374,10 @@ where if opts.bench_benchmarks { // All benchmarks run at the end, in serial. - for b in filtered_benchs { + for (id, b) in filtered_benchs { let event = TestEvent::TeWait(b.desc.clone()); notify_about_test_event(event)?; - run_test(opts, false, b, run_strategy, tx.clone(), Concurrent::No); + run_test(opts, false, id, b, run_strategy, tx.clone(), Concurrent::No); let completed_test = rx.recv().unwrap(); let event = TestEvent::TeResult(completed_test); @@ -448,6 +451,7 @@ pub fn convert_benchmarks_to_tests(tests: Vec) -> Vec, @@ -461,7 +465,7 @@ pub fn run_test( && !cfg!(target_os = "emscripten"); if force_ignore || desc.ignore || ignore_because_no_process_support { - let message = CompletedTest::new(desc, TrIgnored, None, Vec::new()); + let message = CompletedTest::new(id, desc, TrIgnored, None, Vec::new()); monitor_ch.send(message).unwrap(); return None; } @@ -474,6 +478,7 @@ pub fn run_test( } fn run_test_inner( + id: TestId, desc: TestDesc, monitor_ch: Sender, testfn: Box, @@ -484,6 +489,7 @@ pub fn run_test( let runtest = move || match opts.strategy { RunStrategy::InProcess => run_test_in_process( + id, desc, opts.nocapture, opts.time.is_some(), @@ -492,6 +498,7 @@ pub fn run_test( opts.time, ), RunStrategy::SpawnPrimary => spawn_test_subprocess( + id, desc, opts.nocapture, opts.time.is_some(), @@ -530,14 +537,14 @@ pub fn run_test( match testfn { DynBenchFn(bencher) => { // Benchmarks aren't expected to panic, so we run them all in-process. - crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| { + crate::bench::benchmark(id, 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); + crate::bench::benchmark(id, desc, monitor_ch, opts.nocapture, benchfn); None } DynTestFn(f) => { @@ -546,6 +553,7 @@ pub fn run_test( _ => panic!("Cannot run dynamic test fn out-of-process"), }; run_test_inner( + id, desc, monitor_ch, Box::new(move || __rust_begin_short_backtrace(f)), @@ -553,6 +561,7 @@ pub fn run_test( ) } StaticTestFn(f) => run_test_inner( + id, desc, monitor_ch, Box::new(move || __rust_begin_short_backtrace(f)), @@ -571,6 +580,7 @@ fn __rust_begin_short_backtrace(f: F) { } fn run_test_in_process( + id: TestId, desc: TestDesc, nocapture: bool, report_time: bool, @@ -599,11 +609,12 @@ fn run_test_in_process( Err(e) => calc_result(&desc, Err(e.as_ref()), &time_opts, &exec_time), }; let stdout = data.lock().unwrap_or_else(|e| e.into_inner()).to_vec(); - let message = CompletedTest::new(desc, test_result, exec_time, stdout); + let message = CompletedTest::new(id, desc, test_result, exec_time, stdout); monitor_ch.send(message).unwrap(); } fn spawn_test_subprocess( + id: TestId, desc: TestDesc, nocapture: bool, report_time: bool, @@ -653,7 +664,7 @@ fn spawn_test_subprocess( (result, test_output, exec_time) })(); - let message = CompletedTest::new(desc, result, exec_time, test_output); + let message = CompletedTest::new(id, desc, result, exec_time, test_output); monitor_ch.send(message).unwrap(); } diff --git a/library/test/src/tests.rs b/library/test/src/tests.rs index e3c9b3869154a..6a3f31b74ea59 100644 --- a/library/test/src/tests.rs +++ b/library/test/src/tests.rs @@ -94,7 +94,7 @@ pub fn do_not_run_ignored_tests() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_ne!(result, TrOk); } @@ -113,7 +113,7 @@ pub fn ignored_tests_result_in_ignored() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_eq!(result, TrIgnored); } @@ -136,7 +136,7 @@ fn test_should_panic() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_eq!(result, TrOk); } @@ -159,7 +159,7 @@ fn test_should_panic_good_message() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_eq!(result, TrOk); } @@ -187,7 +187,7 @@ fn test_should_panic_bad_message() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_eq!(result, TrFailedMsg(failed_msg.to_string())); } @@ -219,7 +219,7 @@ fn test_should_panic_non_string_message_type() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; assert_eq!(result, TrFailedMsg(failed_msg)); } @@ -243,7 +243,15 @@ fn test_should_panic_but_succeeds() { testfn: DynTestFn(Box::new(f)), }; let (tx, rx) = channel(); - run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test( + &TestOpts::new(), + false, + TestId(0), + desc, + RunStrategy::InProcess, + tx, + Concurrent::No, + ); let result = rx.recv().unwrap().result; assert_eq!( result, @@ -270,7 +278,7 @@ fn report_time_test_template(report_time: bool) -> Option { let test_opts = TestOpts { time_options, ..TestOpts::new() }; let (tx, rx) = channel(); - run_test(&test_opts, false, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let exec_time = rx.recv().unwrap().exec_time; exec_time } @@ -305,7 +313,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, desc, RunStrategy::InProcess, tx, Concurrent::No); + run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No); let result = rx.recv().unwrap().result; result @@ -637,7 +645,7 @@ pub fn test_bench_no_iter() { test_type: TestType::Unknown, }; - crate::bench::benchmark(desc, tx, true, f); + crate::bench::benchmark(TestId(0), desc, tx, true, f); rx.recv().unwrap(); } @@ -657,7 +665,7 @@ pub fn test_bench_iter() { test_type: TestType::Unknown, }; - crate::bench::benchmark(desc, tx, true, f); + crate::bench::benchmark(TestId(0), desc, tx, true, f); rx.recv().unwrap(); } diff --git a/library/test/src/types.rs b/library/test/src/types.rs index 5b75d2f367fff..c5d91f653b356 100644 --- a/library/test/src/types.rs +++ b/library/test/src/types.rs @@ -112,9 +112,13 @@ impl fmt::Debug for TestFn { } } +// A unique integer associated with each test. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct TestId(pub usize); + // The definition of a single test. A test runner will run a list of // these. -#[derive(Clone, Debug, PartialEq, Eq, Hash)] +#[derive(Clone, Debug)] pub struct TestDesc { pub name: TestName, pub ignore: bool,