Skip to content

Test for ambiguous system ordering in CI #13950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/example-run/ambiguity_detection.ron
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
(
)
9 changes: 9 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,7 @@ ron = "0.8.0"
flate2 = "1.0"
serde = { version = "1", features = ["derive"] }
bytemuck = "1.7"
bevy_render = { path = "crates/bevy_render", version = "0.15.0-dev", default-features = false }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an important change, needed to get as much schedules as possible

# Needed to poll Task examples
futures-lite = "2.0.1"
async-std = "1.12"
Expand Down Expand Up @@ -2980,6 +2981,14 @@ description = "Demonstrates customizing default window settings"
category = "Window"
wasm = true

[[example]]
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this is removed? We shouldn't be listing tests as examples TBH.

Copy link
Member Author

@Vrixyz Vrixyz Jul 17, 2024

Choose a reason for hiding this comment

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

That surprised me too, but I kept the behaviour as closely as what's existing (so in my opinion this is out of scope for this Pull Request):

Compare this to next example in Cargo.toml: resizing, which is used as a test, to be noted it has a metadata hidden = true (I believe that's used for the website to know which are published).

If we remove that block, we cannot run the example 😅, mostly a blocker for ci runs.

I believe using examples for testing is a workaround to be guaranteed main thread execution, because if I'm trying to run my example as a test: I have the following error:

$ cargo test --test ambiguity_detection
   Compiling bevy v0.15.0-dev (C:\Users\thier\Documents\perso\bevy)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 17.69s
     Running tests/ecs/ambiguity_detection.rs (C:\Users\thier\Documents\cargo_target_dir\debug\deps\ambiguity_detection-eeb84a8dea010d13.exe)

running 1 test
2024-07-17T14:43:32.965951Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Windows 10 Home", kernel: "19045", cpu: "AMD Ryzen 5 2600X Six-Core Processor", core_count: "6", memory: "32.0 GiB" }
test ambiguity_detection ... FAILED

failures:

---- ambiguity_detection stdout ----
thread 'ambiguity_detection' panicked at C:\Users\thier\.cargo\registry\src\index.crates.io-6f17d22bba15001f\winit-0.30.2\src\platform_impl\windows\event_loop.rs:180:13:   
Initializing the event loop outside of the main thread is a significant cross-platform compatibility hazard. If you absolutely need to create an EventLoop on a different thread, you can use the `EventLoopBuilderExtWindows::any_thread` function.
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Copy link
Member Author

@Vrixyz Vrixyz Jul 17, 2024

Choose a reason for hiding this comment

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

I've tried one last thing, it seems possible to run those as test if we bypass the harness (thanks stackoverflow):

diff --git a/Cargo.toml b/Cargo.toml
index d6eefaa91..533198ce6 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -2981,12 +2981,13 @@ description = "Demonstrates customizing default window settings"
 category = "Window"
 wasm = true
 
-[[example]]
+[[test]]
 name = "ambiguity_detection"
 path = "tests/ecs/ambiguity_detection.rs"
 doc-scrape-examples = true
+harness = false
 
-[package.metadata.example.ambiguity_detection]
+[package.metadata.test.ambiguity_detection]
 hidden = true
 
 [[example]]

I'm not sure about its implications on the rest of bevy machinery though (example scraping, website building, ci tests...), so I'd be happy to study that in a follow up PR rather than in this one :)

name = "ambiguity_detection"
path = "tests/ecs/ambiguity_detection.rs"
doc-scrape-examples = true

[package.metadata.example.ambiguity_detection]
hidden = true

[[example]]
name = "resizing"
path = "tests/window/resizing.rs"
Expand Down
7 changes: 5 additions & 2 deletions crates/bevy_dev_tools/src/ci_testing/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod systems;
pub use self::config::*;

use bevy_app::prelude::*;
use bevy_ecs::schedule::IntoSystemConfigs;
use bevy_time::TimeUpdateStrategy;
use std::time::Duration;

Expand Down Expand Up @@ -45,9 +46,11 @@ impl Plugin for CiTestingPlugin {
fixed_frame_time,
)));
}

app.add_event::<CiTestingCustomEvent>()
.insert_resource(config)
.add_systems(Update, systems::send_events);
.add_systems(
Update,
systems::send_events.before(bevy_window::close_when_requested),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kinda outside the scope of this PR, and now useless as there has been a new conflict on Update...

);
}
}
128 changes: 128 additions & 0 deletions tests/ecs/ambiguity_detection.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
//! A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
//! This is run in CI.

use bevy::{
ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings, ScheduleLabel},
prelude::*,
utils::HashMap,
};
use bevy_render::{pipelined_rendering::RenderExtractApp, Render, RenderApp};

/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
/// so we're just ignoring those for now.
///
/// See [#7386](https://github.com/bevyengine/bevy/issues/7386) for relevant issue.
pub fn get_ignored_ambiguous_system_schedules() -> Vec<Box<dyn ScheduleLabel>> {
vec![
Box::new(First),
Box::new(PreUpdate),
Box::new(Update),
Copy link
Member Author

@Vrixyz Vrixyz Jul 15, 2024

Choose a reason for hiding this comment

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

only 1 ambiguity on Update at the time of testing, probably a good "low-hanging fruit"

2024-07-15T21:35:59.885229Z  WARN update:main app:schedule{name=Main}:schedule{name=Update}: bevy_ecs::schedule::schedule: Schedule Update has ambiguities.
1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- bevy_app::terminal_ctrl_c_handler::TerminalCtrlCHandlerPlugin::exit_on_flag and bevy_dev_tools::ci_testing::systems::send_events
    conflict on: bevy_ecs::world::World

Box::new(PostUpdate),
Box::new(Last),
Box::new(ExtractSchedule),
Box::new(Render),
]
}

/// A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
/// This is run in CI.
pub fn main() {
let mut app = App::new();
app.add_plugins(DefaultPlugins);

let sub_app = app.main_mut();
configure_ambiguity_detection(sub_app);
let sub_app = app.sub_app_mut(RenderApp);
configure_ambiguity_detection(sub_app);
let sub_app = app.sub_app_mut(RenderExtractApp);
configure_ambiguity_detection(sub_app);

app.finish();
app.cleanup();
app.update();

let sub_app = app.main();

let ignored_schedules = get_ignored_ambiguous_system_schedules();

let ambiguities = count_ambiguities(sub_app);
let mut unexpected_ambiguities = vec![];
for (&label, &count) in ambiguities.0.iter() {
if ignored_schedules
.iter()
.any(|label_to_ignore| **label_to_ignore == *label)
{
continue;
}
if count != 0 {
unexpected_ambiguities.push(label);
}
}
assert_eq!(
unexpected_ambiguities.len(),
0,
"Main app has unexpected ambiguities among these schedules: {:?}.\n\
More Details:\n{:#?}",
unexpected_ambiguities,
ambiguities
);

let total_ambiguities = ambiguities.total();
assert_eq!(
total_ambiguities, 82,
"Main app does not have an expected conflicting systems count, \
you might consider verifying if it's normal, or change the expected number.\n\
Details:\n{:#?}",
ambiguities
);

// RenderApp is not checked here, because it is not within the App at this point.
let sub_app = app.sub_app(RenderExtractApp);

let ambiguities = count_ambiguities(sub_app);
let total_ambiguities = ambiguities.total();
assert_eq!(
total_ambiguities, 0,
"RenderExtractApp contains conflicting systems.",
);
}

/// Contains the number of conflicting systems per schedule.
#[derive(Debug, Deref, DerefMut)]
struct AmbiguitiesCount(pub HashMap<InternedScheduleLabel, usize>);

impl AmbiguitiesCount {
fn total(&self) -> usize {
self.values().sum()
}
}

fn configure_ambiguity_detection(sub_app: &mut SubApp) {
let ignored_ambiguous_systems = get_ignored_ambiguous_system_schedules();
let mut schedules = sub_app.world_mut().resource_mut::<Schedules>();
for (_, schedule) in schedules.iter_mut() {
if ignored_ambiguous_systems
.iter()
.any(|label| **label == *schedule.label())
{
// Note: you can remove this bypass to get full details about ambiguities.
continue;
}
schedule.set_build_settings(ScheduleBuildSettings {
ambiguity_detection: LogLevel::Warn,
use_shortnames: false,
..default()
});
}
}

/// Returns the number of conflicting systems per schedule.
fn count_ambiguities(sub_app: &SubApp) -> AmbiguitiesCount {
let schedules = sub_app.world().resource::<Schedules>();
let mut ambiguities = HashMap::new();
for (_, schedule) in schedules.iter() {
let ambiguities_in_schedule = schedule.graph().conflicting_systems().len();
ambiguities.insert(schedule.label(), ambiguities_in_schedule);
}
AmbiguitiesCount(ambiguities)
}