Skip to content

Commit 0c126ee

Browse files
Improve ambiguity detection example / test (#14760)
# Objective While tackling #7386, I noticed a few nits / frustrations with the existing testing infrastructure. Rather than mixing those changes in with much more challenging to review changes to reduce ambiguities, I've split this work into its own PR. ## Solution Substantially simplify the `ambiguity_detection` test code, and reduce the verbosity of logging. ## Testing When run locally, the test functions as expected, with somewhat cleaner logging. --------- Co-authored-by: Jan Hohenheim <[email protected]>
1 parent e9e9e5e commit 0c126ee

File tree

1 file changed

+21
-67
lines changed

1 file changed

+21
-67
lines changed

tests/ecs/ambiguity_detection.rs

Lines changed: 21 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,17 @@
11
//! A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
22
//! This is run in CI.
3+
//!
4+
//! Note that because this test requires rendering, it isn't actually an integration test!
5+
//! Instead, it's secretly an example: you can run this test manually using `cargo run --example ambiguity_detection`.
36
47
use bevy::{
5-
ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings, ScheduleLabel},
8+
ecs::schedule::{InternedScheduleLabel, LogLevel, ScheduleBuildSettings},
69
prelude::*,
710
utils::HashMap,
811
};
9-
use bevy_render::{pipelined_rendering::RenderExtractApp, Render, RenderApp};
12+
use bevy_render::{pipelined_rendering::RenderExtractApp, RenderApp};
1013

11-
/// FIXME: bevy should not have any ambiguities, but it takes time to clean these up,
12-
/// so we're just ignoring those for now.
13-
///
14-
/// See [#7386](https://github.com/bevyengine/bevy/issues/7386) for relevant issue.
15-
pub fn get_ignored_ambiguous_system_schedules() -> Vec<Box<dyn ScheduleLabel>> {
16-
vec![
17-
Box::new(First),
18-
Box::new(PreUpdate),
19-
Box::new(Update),
20-
Box::new(PostUpdate),
21-
Box::new(Last),
22-
Box::new(ExtractSchedule),
23-
Box::new(Render),
24-
]
25-
}
26-
27-
/// A test to confirm that `bevy` doesn't regress its system ambiguities count when using [`DefaultPlugins`].
28-
/// This is run in CI.
29-
pub fn main() {
14+
fn main() {
3015
let mut app = App::new();
3116
app.add_plugins(DefaultPlugins);
3217

@@ -41,49 +26,25 @@ pub fn main() {
4126
app.cleanup();
4227
app.update();
4328

44-
let sub_app = app.main();
45-
46-
let ignored_schedules = get_ignored_ambiguous_system_schedules();
47-
48-
let ambiguities = count_ambiguities(sub_app);
49-
let mut unexpected_ambiguities = vec![];
50-
for (&label, &count) in ambiguities.0.iter() {
51-
if ignored_schedules
52-
.iter()
53-
.any(|label_to_ignore| **label_to_ignore == *label)
54-
{
55-
continue;
56-
}
57-
if count != 0 {
58-
unexpected_ambiguities.push(label);
59-
}
60-
}
29+
let main_app_ambiguities = count_ambiguities(app.main());
6130
assert_eq!(
62-
unexpected_ambiguities.len(),
63-
0,
64-
"Main app has unexpected ambiguities among these schedules: {:?}.\n\
65-
More Details:\n{:#?}",
66-
unexpected_ambiguities,
67-
ambiguities
68-
);
69-
70-
let total_ambiguities = ambiguities.total();
71-
assert_eq!(
72-
total_ambiguities, 72,
73-
"Main app does not have an expected conflicting systems count, \
74-
you might consider verifying if it's normal, or change the expected number.\n\
75-
Details:\n{:#?}",
76-
ambiguities
31+
main_app_ambiguities.total(),
32+
// This number *should* be zero.
33+
// Over time, we are working to reduce the number: your PR should not increase it.
34+
// If you decrease this by fixing an ambiguity, reduce the number to prevent regressions.
35+
// See https://github.com/bevyengine/bevy/issues/7386 for progress.
36+
72,
37+
"Main app has unexpected ambiguities among the following schedules: \n{:#?}.",
38+
main_app_ambiguities,
7739
);
7840

7941
// RenderApp is not checked here, because it is not within the App at this point.
80-
let sub_app = app.sub_app(RenderExtractApp);
81-
82-
let ambiguities = count_ambiguities(sub_app);
83-
let total_ambiguities = ambiguities.total();
42+
let render_extract_ambiguities = count_ambiguities(app.sub_app(RenderExtractApp));
8443
assert_eq!(
85-
total_ambiguities, 0,
86-
"RenderExtractApp contains conflicting systems.",
44+
render_extract_ambiguities.total(),
45+
0,
46+
"RenderExtract app has unexpected ambiguities among the following schedules: \n{:#?}",
47+
render_extract_ambiguities,
8748
);
8849
}
8950

@@ -98,17 +59,10 @@ impl AmbiguitiesCount {
9859
}
9960

10061
fn configure_ambiguity_detection(sub_app: &mut SubApp) {
101-
let ignored_ambiguous_systems = get_ignored_ambiguous_system_schedules();
10262
let mut schedules = sub_app.world_mut().resource_mut::<Schedules>();
10363
for (_, schedule) in schedules.iter_mut() {
104-
if ignored_ambiguous_systems
105-
.iter()
106-
.any(|label| **label == *schedule.label())
107-
{
108-
// Note: you can remove this bypass to get full details about ambiguities.
109-
continue;
110-
}
11164
schedule.set_build_settings(ScheduleBuildSettings {
65+
// NOTE: you can change this to `LogLevel::Ignore` to easily see the current number of ambiguities.
11266
ambiguity_detection: LogLevel::Warn,
11367
use_shortnames: false,
11468
..default()

0 commit comments

Comments
 (0)