Skip to content

Commit 5e73bd1

Browse files
borsbors
committed
Auto merge of #82300 - andersk:libtest-id, r=Amanieu
libtest: Index tests by a unique TestId This more robustly avoids problems with duplicate `TestDesc`. See #81852 and #82274. Cc `@Mark-Simulacrum.`
2 parents c18c0ad + 3c42d9f commit 5e73bd1

File tree

5 files changed

+75
-41
lines changed

5 files changed

+75
-41
lines changed

‎library/test/src/bench.rs

+13-4
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
pub use std::hint::black_box;
33

44
use super::{
5-
event::CompletedTest, options::BenchMode, test_result::TestResult, types::TestDesc, Sender,
5+
event::CompletedTest,
6+
options::BenchMode,
7+
test_result::TestResult,
8+
types::{TestDesc, TestId},
9+
Sender,
610
};
711

812
use crate::stats;
@@ -177,8 +181,13 @@ where
177181
}
178182
}
179183

180-
pub fn benchmark<F>(desc: TestDesc, monitor_ch: Sender<CompletedTest>, nocapture: bool, f: F)
181-
where
184+
pub fn benchmark<F>(
185+
id: TestId,
186+
desc: TestDesc,
187+
monitor_ch: Sender<CompletedTest>,
188+
nocapture: bool,
189+
f: F,
190+
) where
182191
F: FnMut(&mut Bencher),
183192
{
184193
let mut bs = Bencher { mode: BenchMode::Auto, summary: None, bytes: 0 };
@@ -213,7 +222,7 @@ where
213222
};
214223

215224
let stdout = data.lock().unwrap().to_vec();
216-
let message = CompletedTest::new(desc, test_result, None, stdout);
225+
let message = CompletedTest::new(id, desc, test_result, None, stdout);
217226
monitor_ch.send(message).unwrap();
218227
}
219228

‎library/test/src/event.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@
33
44
use super::test_result::TestResult;
55
use super::time::TestExecTime;
6-
use super::types::TestDesc;
6+
use super::types::{TestDesc, TestId};
77

88
#[derive(Debug, Clone)]
99
pub struct CompletedTest {
10+
pub id: TestId,
1011
pub desc: TestDesc,
1112
pub result: TestResult,
1213
pub exec_time: Option<TestExecTime>,
@@ -15,12 +16,13 @@ pub struct CompletedTest {
1516

1617
impl CompletedTest {
1718
pub fn new(
19+
id: TestId,
1820
desc: TestDesc,
1921
result: TestResult,
2022
exec_time: Option<TestExecTime>,
2123
stdout: Vec<u8>,
2224
) -> Self {
23-
Self { desc, result, exec_time, stdout }
25+
Self { id, desc, result, exec_time, stdout }
2426
}
2527
}
2628

‎library/test/src/lib.rs

+34-23
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ pub mod test {
5454
time::{TestExecTime, TestTimeOptions},
5555
types::{
5656
DynTestFn, DynTestName, StaticBenchFn, StaticTestFn, StaticTestName, TestDesc,
57-
TestDescAndFn, TestName, TestType,
57+
TestDescAndFn, TestId, TestName, TestType,
5858
},
5959
};
6060
}
@@ -215,9 +215,10 @@ where
215215

216216
// Use a deterministic hasher
217217
type TestMap =
218-
HashMap<TestDesc, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
218+
HashMap<TestId, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
219219

220220
struct TimeoutEntry {
221+
id: TestId,
221222
desc: TestDesc,
222223
timeout: Instant,
223224
}
@@ -249,7 +250,9 @@ where
249250

250251
let (filtered_tests, filtered_benchs): (Vec<_>, _) = filtered_tests
251252
.into_iter()
252-
.partition(|e| matches!(e.testfn, StaticTestFn(_) | DynTestFn(_)));
253+
.enumerate()
254+
.map(|(i, e)| (TestId(i), e))
255+
.partition(|(_, e)| matches!(e.testfn, StaticTestFn(_) | DynTestFn(_)));
253256

254257
let concurrency = opts.test_threads.unwrap_or_else(get_concurrency);
255258

@@ -278,7 +281,7 @@ where
278281
break;
279282
}
280283
let timeout_entry = timeout_queue.pop_front().unwrap();
281-
if running_tests.contains_key(&timeout_entry.desc) {
284+
if running_tests.contains_key(&timeout_entry.id) {
282285
timed_out.push(timeout_entry.desc);
283286
}
284287
}
@@ -294,11 +297,11 @@ where
294297

295298
if concurrency == 1 {
296299
while !remaining.is_empty() {
297-
let test = remaining.pop().unwrap();
300+
let (id, test) = remaining.pop().unwrap();
298301
let event = TestEvent::TeWait(test.desc.clone());
299302
notify_about_test_event(event)?;
300303
let join_handle =
301-
run_test(opts, !opts.run_tests, test, run_strategy, tx.clone(), Concurrent::No);
304+
run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone(), Concurrent::No);
302305
assert!(join_handle.is_none());
303306
let completed_test = rx.recv().unwrap();
304307

@@ -308,7 +311,7 @@ where
308311
} else {
309312
while pending > 0 || !remaining.is_empty() {
310313
while pending < concurrency && !remaining.is_empty() {
311-
let test = remaining.pop().unwrap();
314+
let (id, test) = remaining.pop().unwrap();
312315
let timeout = time::get_default_test_timeout();
313316
let desc = test.desc.clone();
314317

@@ -317,13 +320,14 @@ where
317320
let join_handle = run_test(
318321
opts,
319322
!opts.run_tests,
323+
id,
320324
test,
321325
run_strategy,
322326
tx.clone(),
323327
Concurrent::Yes,
324328
);
325-
running_tests.insert(desc.clone(), RunningTest { join_handle });
326-
timeout_queue.push_back(TimeoutEntry { desc, timeout });
329+
running_tests.insert(id, RunningTest { join_handle });
330+
timeout_queue.push_back(TimeoutEntry { id, desc, timeout });
327331
pending += 1;
328332
}
329333

@@ -352,13 +356,12 @@ where
352356
}
353357

354358
let mut completed_test = res.unwrap();
355-
if let Some(running_test) = running_tests.remove(&completed_test.desc) {
356-
if let Some(join_handle) = running_test.join_handle {
357-
if let Err(_) = join_handle.join() {
358-
if let TrOk = completed_test.result {
359-
completed_test.result =
360-
TrFailedMsg("panicked after reporting success".to_string());
361-
}
359+
let running_test = running_tests.remove(&completed_test.id).unwrap();
360+
if let Some(join_handle) = running_test.join_handle {
361+
if let Err(_) = join_handle.join() {
362+
if let TrOk = completed_test.result {
363+
completed_test.result =
364+
TrFailedMsg("panicked after reporting success".to_string());
362365
}
363366
}
364367
}
@@ -371,10 +374,10 @@ where
371374

372375
if opts.bench_benchmarks {
373376
// All benchmarks run at the end, in serial.
374-
for b in filtered_benchs {
377+
for (id, b) in filtered_benchs {
375378
let event = TestEvent::TeWait(b.desc.clone());
376379
notify_about_test_event(event)?;
377-
run_test(opts, false, b, run_strategy, tx.clone(), Concurrent::No);
380+
run_test(opts, false, id, b, run_strategy, tx.clone(), Concurrent::No);
378381
let completed_test = rx.recv().unwrap();
379382

380383
let event = TestEvent::TeResult(completed_test);
@@ -448,6 +451,7 @@ pub fn convert_benchmarks_to_tests(tests: Vec<TestDescAndFn>) -> Vec<TestDescAnd
448451
pub fn run_test(
449452
opts: &TestOpts,
450453
force_ignore: bool,
454+
id: TestId,
451455
test: TestDescAndFn,
452456
strategy: RunStrategy,
453457
monitor_ch: Sender<CompletedTest>,
@@ -461,7 +465,7 @@ pub fn run_test(
461465
&& !cfg!(target_os = "emscripten");
462466

463467
if force_ignore || desc.ignore || ignore_because_no_process_support {
464-
let message = CompletedTest::new(desc, TrIgnored, None, Vec::new());
468+
let message = CompletedTest::new(id, desc, TrIgnored, None, Vec::new());
465469
monitor_ch.send(message).unwrap();
466470
return None;
467471
}
@@ -474,6 +478,7 @@ pub fn run_test(
474478
}
475479

476480
fn run_test_inner(
481+
id: TestId,
477482
desc: TestDesc,
478483
monitor_ch: Sender<CompletedTest>,
479484
testfn: Box<dyn FnOnce() + Send>,
@@ -484,6 +489,7 @@ pub fn run_test(
484489

485490
let runtest = move || match opts.strategy {
486491
RunStrategy::InProcess => run_test_in_process(
492+
id,
487493
desc,
488494
opts.nocapture,
489495
opts.time.is_some(),
@@ -492,6 +498,7 @@ pub fn run_test(
492498
opts.time,
493499
),
494500
RunStrategy::SpawnPrimary => spawn_test_subprocess(
501+
id,
495502
desc,
496503
opts.nocapture,
497504
opts.time.is_some(),
@@ -530,14 +537,14 @@ pub fn run_test(
530537
match testfn {
531538
DynBenchFn(bencher) => {
532539
// Benchmarks aren't expected to panic, so we run them all in-process.
533-
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, |harness| {
540+
crate::bench::benchmark(id, desc, monitor_ch, opts.nocapture, |harness| {
534541
bencher.run(harness)
535542
});
536543
None
537544
}
538545
StaticBenchFn(benchfn) => {
539546
// Benchmarks aren't expected to panic, so we run them all in-process.
540-
crate::bench::benchmark(desc, monitor_ch, opts.nocapture, benchfn);
547+
crate::bench::benchmark(id, desc, monitor_ch, opts.nocapture, benchfn);
541548
None
542549
}
543550
DynTestFn(f) => {
@@ -546,13 +553,15 @@ pub fn run_test(
546553
_ => panic!("Cannot run dynamic test fn out-of-process"),
547554
};
548555
run_test_inner(
556+
id,
549557
desc,
550558
monitor_ch,
551559
Box::new(move || __rust_begin_short_backtrace(f)),
552560
test_run_opts,
553561
)
554562
}
555563
StaticTestFn(f) => run_test_inner(
564+
id,
556565
desc,
557566
monitor_ch,
558567
Box::new(move || __rust_begin_short_backtrace(f)),
@@ -571,6 +580,7 @@ fn __rust_begin_short_backtrace<F: FnOnce()>(f: F) {
571580
}
572581

573582
fn run_test_in_process(
583+
id: TestId,
574584
desc: TestDesc,
575585
nocapture: bool,
576586
report_time: bool,
@@ -599,11 +609,12 @@ fn run_test_in_process(
599609
Err(e) => calc_result(&desc, Err(e.as_ref()), &time_opts, &exec_time),
600610
};
601611
let stdout = data.lock().unwrap_or_else(|e| e.into_inner()).to_vec();
602-
let message = CompletedTest::new(desc, test_result, exec_time, stdout);
612+
let message = CompletedTest::new(id, desc, test_result, exec_time, stdout);
603613
monitor_ch.send(message).unwrap();
604614
}
605615

606616
fn spawn_test_subprocess(
617+
id: TestId,
607618
desc: TestDesc,
608619
nocapture: bool,
609620
report_time: bool,
@@ -653,7 +664,7 @@ fn spawn_test_subprocess(
653664
(result, test_output, exec_time)
654665
})();
655666

656-
let message = CompletedTest::new(desc, result, exec_time, test_output);
667+
let message = CompletedTest::new(id, desc, result, exec_time, test_output);
657668
monitor_ch.send(message).unwrap();
658669
}
659670

‎library/test/src/tests.rs

+19-11
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ pub fn do_not_run_ignored_tests() {
9494
testfn: DynTestFn(Box::new(f)),
9595
};
9696
let (tx, rx) = channel();
97-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
97+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
9898
let result = rx.recv().unwrap().result;
9999
assert_ne!(result, TrOk);
100100
}
@@ -113,7 +113,7 @@ pub fn ignored_tests_result_in_ignored() {
113113
testfn: DynTestFn(Box::new(f)),
114114
};
115115
let (tx, rx) = channel();
116-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
116+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
117117
let result = rx.recv().unwrap().result;
118118
assert_eq!(result, TrIgnored);
119119
}
@@ -136,7 +136,7 @@ fn test_should_panic() {
136136
testfn: DynTestFn(Box::new(f)),
137137
};
138138
let (tx, rx) = channel();
139-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
139+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
140140
let result = rx.recv().unwrap().result;
141141
assert_eq!(result, TrOk);
142142
}
@@ -159,7 +159,7 @@ fn test_should_panic_good_message() {
159159
testfn: DynTestFn(Box::new(f)),
160160
};
161161
let (tx, rx) = channel();
162-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
162+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
163163
let result = rx.recv().unwrap().result;
164164
assert_eq!(result, TrOk);
165165
}
@@ -187,7 +187,7 @@ fn test_should_panic_bad_message() {
187187
testfn: DynTestFn(Box::new(f)),
188188
};
189189
let (tx, rx) = channel();
190-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
190+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
191191
let result = rx.recv().unwrap().result;
192192
assert_eq!(result, TrFailedMsg(failed_msg.to_string()));
193193
}
@@ -219,7 +219,7 @@ fn test_should_panic_non_string_message_type() {
219219
testfn: DynTestFn(Box::new(f)),
220220
};
221221
let (tx, rx) = channel();
222-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
222+
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
223223
let result = rx.recv().unwrap().result;
224224
assert_eq!(result, TrFailedMsg(failed_msg));
225225
}
@@ -243,7 +243,15 @@ fn test_should_panic_but_succeeds() {
243243
testfn: DynTestFn(Box::new(f)),
244244
};
245245
let (tx, rx) = channel();
246-
run_test(&TestOpts::new(), false, desc, RunStrategy::InProcess, tx, Concurrent::No);
246+
run_test(
247+
&TestOpts::new(),
248+
false,
249+
TestId(0),
250+
desc,
251+
RunStrategy::InProcess,
252+
tx,
253+
Concurrent::No,
254+
);
247255
let result = rx.recv().unwrap().result;
248256
assert_eq!(
249257
result,
@@ -270,7 +278,7 @@ fn report_time_test_template(report_time: bool) -> Option<TestExecTime> {
270278

271279
let test_opts = TestOpts { time_options, ..TestOpts::new() };
272280
let (tx, rx) = channel();
273-
run_test(&test_opts, false, desc, RunStrategy::InProcess, tx, Concurrent::No);
281+
run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
274282
let exec_time = rx.recv().unwrap().exec_time;
275283
exec_time
276284
}
@@ -305,7 +313,7 @@ fn time_test_failure_template(test_type: TestType) -> TestResult {
305313

306314
let test_opts = TestOpts { time_options: Some(time_options), ..TestOpts::new() };
307315
let (tx, rx) = channel();
308-
run_test(&test_opts, false, desc, RunStrategy::InProcess, tx, Concurrent::No);
316+
run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
309317
let result = rx.recv().unwrap().result;
310318

311319
result
@@ -637,7 +645,7 @@ pub fn test_bench_no_iter() {
637645
test_type: TestType::Unknown,
638646
};
639647

640-
crate::bench::benchmark(desc, tx, true, f);
648+
crate::bench::benchmark(TestId(0), desc, tx, true, f);
641649
rx.recv().unwrap();
642650
}
643651

@@ -657,7 +665,7 @@ pub fn test_bench_iter() {
657665
test_type: TestType::Unknown,
658666
};
659667

660-
crate::bench::benchmark(desc, tx, true, f);
668+
crate::bench::benchmark(TestId(0), desc, tx, true, f);
661669
rx.recv().unwrap();
662670
}
663671

0 commit comments

Comments
 (0)