Skip to content

Commit

Permalink
Fix bug in parallel test execution
Browse files Browse the repository at this point in the history
Thanks @hanna-kruppe for noticing
  • Loading branch information
LukasKalbertodt committed Oct 5, 2024
1 parent 7413feb commit d6ac84f
Showing 1 changed file with 8 additions and 5 deletions.
13 changes: 8 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,11 +519,14 @@ pub fn run(args: &Arguments, mut tests: Vec<Trial>) -> Conclusion {
// Start worker threads
for _ in 0..num_threads {
scope.spawn(|| {
// Get next test to process from the iterator. We have the
// extra `let` binding as otherwise, the mutex would be
// locked for the whole `if` body.
let next_trial = iter.lock().unwrap().next();
if let Some(trial) = next_trial {
loop {
// Get next test to process from the iterator. We have the
// extra `let` binding as otherwise, the mutex would be
// locked for the whole `if` body.
let Some(trial) = iter.lock().unwrap().next() else {
break;
};

let payload = if args.is_ignored(&trial) {
(Outcome::Ignored, trial.info)
} else {
Expand Down

1 comment on commit d6ac84f

@hanna-kruppe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is outdated now, it should say something about not holding the lock while the test is running. I am never 100% sure about when temporaries get dropped but empirically this version seems to work. I tested this commit in a workspace of mine with more tests than threads, all of them ran and it used multiple threads as expected. I also don't see any other problems at a glance.

Please sign in to comment.