From 3217f216aaf18e855ad925079098c6c0535cf55d Mon Sep 17 00:00:00 2001
From: Mike <mike.hsu@gmail.com>
Date: Mon, 20 Jun 2022 17:35:55 +0000
Subject: [PATCH] change panicking test to not run on global task pool (#4998)

# Objective

- Fixes #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.
---
 crates/bevy_transform/src/systems.rs | 29 ++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/crates/bevy_transform/src/systems.rs b/crates/bevy_transform/src/systems.rs
index e7bd9789d3..672e67b513 100644
--- a/crates/bevy_transform/src/systems.rs
+++ b/crates/bevy_transform/src/systems.rs
@@ -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);
@@ -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);
@@ -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);
@@ -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);
 
@@ -339,19 +339,20 @@ 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(),
@@ -359,13 +360,13 @@ mod test {
                 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);
     }
 }