Skip to content

Commit

Permalink
Rollup merge of #103681 - RalfJung:libtest-thread, r=thomcc
Browse files Browse the repository at this point in the history 
libtest: run all tests in their own thread, if supported by the host

This reverts the threading changes of rust-lang/rust#56243, which made it so that with `-j1`, the test harness does not spawn any threads. Those changes were done to enable Miri to run the test harness, but Miri supports threads nowadays, so this is no longer needed. Using a thread for each test is useful because the thread's name can be set to the test's name which makes panic messages consistent between `-j1` and `-j2` runs and also a bit more readable.

I did not revert the HashMap changes of rust-lang/rust#56243; using a deterministic map seems fine for the test harness and the more deterministic testing is the better.

Fixes rust-lang/rust#59122
Fixes rust-lang/rust#70492
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Nov 4, 2022
2 parents b2af3d1 + 708d5c8 commit 4034aec
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 54 deletions.
58 changes: 28 additions & 30 deletions test/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub mod test {
cli::{parse_opts, TestOpts},
filter_tests,
helpers::metrics::{Metric, MetricMap},
options::{Concurrent, Options, RunIgnored, RunStrategy, ShouldPanic},
options::{Options, RunIgnored, RunStrategy, ShouldPanic},
run_test, test_main, test_main_static,
test_result::{TestResult, TrFailed, TrFailedMsg, TrIgnored, TrOk},
time::{TestExecTime, TestTimeOptions},
Expand Down Expand Up @@ -85,7 +85,7 @@ use event::{CompletedTest, TestEvent};
use helpers::concurrency::get_concurrency;
use helpers::exit_code::get_exit_code;
use helpers::shuffle::{get_shuffle_seed, shuffle_tests};
use options::{Concurrent, RunStrategy};
use options::RunStrategy;
use test_result::*;
use time::TestExecTime;

Expand Down Expand Up @@ -267,6 +267,19 @@ where
join_handle: Option<thread::JoinHandle<()>>,
}

impl RunningTest {
fn join(self, completed_test: &mut CompletedTest) {
if let Some(join_handle) = self.join_handle {
if let Err(_) = join_handle.join() {
if let TrOk = completed_test.result {
completed_test.result =
TrFailedMsg("panicked after reporting success".to_string());
}
}
}
}
}

// Use a deterministic hasher
type TestMap =
HashMap<TestId, RunningTest, BuildHasherDefault<collections::hash_map::DefaultHasher>>;
Expand Down Expand Up @@ -366,10 +379,10 @@ where
let (id, test) = remaining.pop_front().unwrap();
let event = TestEvent::TeWait(test.desc.clone());
notify_about_test_event(event)?;
let join_handle =
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();
let join_handle = run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone());
// Wait for the test to complete.
let mut completed_test = rx.recv().unwrap();
RunningTest { join_handle }.join(&mut completed_test);

let event = TestEvent::TeResult(completed_test);
notify_about_test_event(event)?;
Expand All @@ -383,15 +396,8 @@ where

let event = TestEvent::TeWait(desc.clone());
notify_about_test_event(event)?; //here no pad
let join_handle = run_test(
opts,
!opts.run_tests,
id,
test,
run_strategy,
tx.clone(),
Concurrent::Yes,
);
let join_handle =
run_test(opts, !opts.run_tests, id, test, run_strategy, tx.clone());
running_tests.insert(id, RunningTest { join_handle });
timeout_queue.push_back(TimeoutEntry { id, desc, timeout });
pending += 1;
Expand Down Expand Up @@ -423,14 +429,7 @@ where

let mut completed_test = res.unwrap();
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());
}
}
}
running_test.join(&mut completed_test);

let event = TestEvent::TeResult(completed_test);
notify_about_test_event(event)?;
Expand All @@ -443,8 +442,10 @@ where
for (id, b) in filtered.benchs {
let event = TestEvent::TeWait(b.desc.clone());
notify_about_test_event(event)?;
run_test(opts, false, id, b, run_strategy, tx.clone(), Concurrent::No);
let completed_test = rx.recv().unwrap();
let join_handle = run_test(opts, false, id, b, run_strategy, tx.clone());
// Wait for the test to complete.
let mut completed_test = rx.recv().unwrap();
RunningTest { join_handle }.join(&mut completed_test);

let event = TestEvent::TeResult(completed_test);
notify_about_test_event(event)?;
Expand Down Expand Up @@ -520,7 +521,6 @@ pub fn run_test(
test: TestDescAndFn,
strategy: RunStrategy,
monitor_ch: Sender<CompletedTest>,
concurrency: Concurrent,
) -> Option<thread::JoinHandle<()>> {
let TestDescAndFn { desc, testfn } = test;

Expand All @@ -538,7 +538,6 @@ pub fn run_test(
struct TestRunOpts {
pub strategy: RunStrategy,
pub nocapture: bool,
pub concurrency: Concurrent,
pub time: Option<time::TestTimeOptions>,
}

Expand All @@ -549,7 +548,6 @@ pub fn run_test(
testfn: Box<dyn FnOnce() -> Result<(), String> + Send>,
opts: TestRunOpts,
) -> Option<thread::JoinHandle<()>> {
let concurrency = opts.concurrency;
let name = desc.name.clone();

let runtest = move || match opts.strategy {
Expand All @@ -576,7 +574,7 @@ pub fn run_test(
// the test synchronously, regardless of the concurrency
// level.
let supports_threads = !cfg!(target_os = "emscripten") && !cfg!(target_family = "wasm");
if concurrency == Concurrent::Yes && supports_threads {
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();
Expand All @@ -597,7 +595,7 @@ pub fn run_test(
}

let test_run_opts =
TestRunOpts { strategy, nocapture: opts.nocapture, concurrency, time: opts.time_options };
TestRunOpts { strategy, nocapture: opts.nocapture, time: opts.time_options };

match testfn {
DynBenchFn(benchfn) => {
Expand Down
7 changes: 0 additions & 7 deletions test/src/options.rs
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,5 @@
//! Enums denoting options for test execution.

/// Whether to execute tests concurrently or not
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub enum Concurrent {
Yes,
No,
}

/// Number of times to run a benchmarked function
#[derive(Clone, PartialEq, Eq)]
pub enum BenchMode {
Expand Down
26 changes: 9 additions & 17 deletions test/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ pub fn do_not_run_ignored_tests() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_ne!(result, TrOk);
}
Expand All @@ -125,7 +125,7 @@ pub fn ignored_tests_result_in_ignored() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(result, TrIgnored);
}
Expand All @@ -150,7 +150,7 @@ fn test_should_panic() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(result, TrOk);
}
Expand All @@ -175,7 +175,7 @@ fn test_should_panic_good_message() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(result, TrOk);
}
Expand Down Expand Up @@ -205,7 +205,7 @@ fn test_should_panic_bad_message() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(result, TrFailedMsg(failed_msg.to_string()));
}
Expand Down Expand Up @@ -239,7 +239,7 @@ fn test_should_panic_non_string_message_type() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(result, TrFailedMsg(failed_msg));
}
Expand Down Expand Up @@ -267,15 +267,7 @@ fn test_should_panic_but_succeeds() {
testfn: DynTestFn(Box::new(f)),
};
let (tx, rx) = channel();
run_test(
&TestOpts::new(),
false,
TestId(0),
desc,
RunStrategy::InProcess,
tx,
Concurrent::No,
);
run_test(&TestOpts::new(), false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;
assert_eq!(
result,
Expand Down Expand Up @@ -306,7 +298,7 @@ fn report_time_test_template(report_time: bool) -> Option<TestExecTime> {

let test_opts = TestOpts { time_options, ..TestOpts::new() };
let (tx, rx) = channel();
run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx);
let exec_time = rx.recv().unwrap().exec_time;
exec_time
}
Expand Down Expand Up @@ -345,7 +337,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, TestId(0), desc, RunStrategy::InProcess, tx, Concurrent::No);
run_test(&test_opts, false, TestId(0), desc, RunStrategy::InProcess, tx);
let result = rx.recv().unwrap().result;

result
Expand Down

0 comments on commit 4034aec

Please sign in to comment.