Skip to content

Commit

Permalink
change panicking test to not run on global task pool (bevyengine#4998)
Browse files Browse the repository at this point in the history
# Objective

- Fixes bevyengine#4996 

## Solution

- Panicking on the global task pool is probably bad. This changes the panicking test to use a single threaded stage to run the test instead.
- I checked the other #[should_panic]
- I also added explicit ordering between the transform propagate system and the parent update system. The ambiguous ordering didn't seem to be causing problems, but the tests are probably more correct this way. The plugins that add these systems have an explicit ordering. I can remove this if necessary.

## Note

I don't have a 100% mental model of why panicking is causing intermittent failures. It probably has to do with a task for one of the other tests landing on the panicking thread when it actually panics. Why this causes a problem I'm not sure, but this PR seems to fix things.

## Open questions

- there are some other #[should_panic] tests that run on the task pool in stage.rs. I don't think we restart panicked threads, so this might be killing most of the threads on the pool. But since they're not causing test failures, we should probably decide what to do about that separately. The solution in this PR won't work since those tests are explicitly testing parallelism.
  • Loading branch information
hymm authored and james7132 committed Jun 22, 2022
1 parent c5df0d6 commit d5a5993
Showing 1 changed file with 15 additions and 14 deletions.
29 changes: 15 additions & 14 deletions crates/bevy_transform/src/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ mod test {

let mut update_stage = SystemStage::parallel();
update_stage.add_system(parent_update_system);
update_stage.add_system(transform_propagate_system);
update_stage.add_system(transform_propagate_system.after(parent_update_system));

let mut schedule = Schedule::default();
schedule.add_stage("update", update_stage);
Expand Down Expand Up @@ -158,7 +158,7 @@ mod test {

let mut update_stage = SystemStage::parallel();
update_stage.add_system(parent_update_system);
update_stage.add_system(transform_propagate_system);
update_stage.add_system(transform_propagate_system.after(parent_update_system));

let mut schedule = Schedule::default();
schedule.add_stage("update", update_stage);
Expand Down Expand Up @@ -201,7 +201,7 @@ mod test {

let mut update_stage = SystemStage::parallel();
update_stage.add_system(parent_update_system);
update_stage.add_system(transform_propagate_system);
update_stage.add_system(transform_propagate_system.after(parent_update_system));

let mut schedule = Schedule::default();
schedule.add_stage("update", update_stage);
Expand Down Expand Up @@ -286,7 +286,7 @@ mod test {
let mut app = App::new();

app.add_system(parent_update_system);
app.add_system(transform_propagate_system);
app.add_system(transform_propagate_system.after(parent_update_system));

let translation = vec3(1.0, 0.0, 0.0);

Expand Down Expand Up @@ -339,33 +339,34 @@ mod test {
#[test]
#[should_panic]
fn panic_when_hierarchy_cycle() {
let mut app = App::new();
let mut world = World::default();
// This test is run on a single thread in order to avoid breaking the global task pool by panicking
// This fixes the flaky tests reported in https://github.com/bevyengine/bevy/issues/4996
let mut update_stage = SystemStage::single_threaded();

app.add_system(parent_update_system);
app.add_system(transform_propagate_system);
update_stage.add_system(parent_update_system);
update_stage.add_system(transform_propagate_system.after(parent_update_system));

let child = app
.world
let child = world
.spawn()
.insert_bundle((Transform::identity(), GlobalTransform::default()))
.id();

let grandchild = app
.world
let grandchild = world
.spawn()
.insert_bundle((
Transform::identity(),
GlobalTransform::default(),
Parent(child),
))
.id();
app.world.spawn().insert_bundle((
world.spawn().insert_bundle((
Transform::default(),
GlobalTransform::default(),
Children::with(&[child]),
));
app.world.entity_mut(child).insert(Parent(grandchild));
world.entity_mut(child).insert(Parent(grandchild));

app.update();
update_stage.run(&mut world);
}
}

0 comments on commit d5a5993

Please sign in to comment.