Skip to content
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

Add test plugin group #5932

Closed
wants to merge 2 commits into from

Conversation

anchpop
Copy link
Contributor

@anchpop anchpop commented Sep 9, 2022

Objective

Work towards closing #5931

Solution

  • Add a TestPlugins group that adds MinimalPlugins, AssetPlugin, ScenePlugin, WindowPlugin, GilrsPlugin, TransformPlugin, HierarchyPlugin, DiagnosticsPlugin, and InputPlugin.
  • Added an example for how to use it. The example gets run in CI, which should make sure that no future change ever breaks any of the plugins in Github Actions

Limitations


Changelog

Added

TestPlugins group that adds plugins suitable for use in automated tests.

@anchpop anchpop marked this pull request as ready for review September 9, 2022 21:20
@anchpop anchpop marked this pull request as draft September 9, 2022 21:28
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Diagnostics Logging, crash handling, error reporting and performance analysis labels Sep 9, 2022
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is a good idea, and I agree with this selection.

@mockersf
Copy link
Member

mockersf commented Sep 9, 2022

I would rather have a feature that would enable only what is interesting in CI than a plugin group. The feature would help reducing compile time.

@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 2 times, most recently from e0c042c to a5412b2 Compare September 9, 2022 21:38
@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch from a5412b2 to 0740263 Compare September 9, 2022 21:46
@anchpop
Copy link
Contributor Author

anchpop commented Sep 9, 2022

I would rather have a feature that would enable only what is interesting in CI than a plugin group. The feature would help reducing compile time.

Any tests that run can't have WinitPlugin, and as far as I can tell there's no way to tell cargo to automatically compile bevy with different features when running cargo test. Even if there were, I want to leave open the possibility of having a custom test harness that allows you to re-run failing tests using DefaultPlugins so you can see what's going on.

@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 2 times, most recently from 1172d13 to 9b57fc3 Compare September 9, 2022 22:51
@anchpop anchpop marked this pull request as ready for review September 9, 2022 22:56
@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch from 9b57fc3 to 7db32f6 Compare September 9, 2022 23:12
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

The example should live in the root level tests directory, which will avoid the shenanigans :) We should try and publicize that better though; perhaps we can add a note to the examples README?

@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 3 times, most recently from b7c6edb to f51258c Compare September 10, 2022 02:06
@anchpop
Copy link
Contributor Author

anchpop commented Sep 10, 2022

The example should live in the root level tests directory, which will avoid the shenanigans :) We should try and publicize that better though; perhaps we can add a note to the examples README?

Done. There was already how_to_test_systems.rs, but I kept my example because it's much simpler than the other one.

@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 2 times, most recently from 2f0c2b6 to d2e056d Compare September 10, 2022 02:19
@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 2 times, most recently from 7d89248 to d448cc8 Compare September 10, 2022 02:31
@@ -0,0 +1,48 @@
//! This example illustrates test systems.
fn main() {
Copy link
Member

Choose a reason for hiding this comment

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

The entire main function shouldn't be necessary here.

Copy link
Contributor Author

@anchpop anchpop Sep 10, 2022

Choose a reason for hiding this comment

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

Since I'm linking to it in the examples page, I wanted to give an informative message to someone who accidentally ran cargo run --example automated_tests rather than cargo test --example automated_tests

@alice-i-cecile
Copy link
Member

@mockersf I want your opinion on the organization here. I think it's interesting that we're linking to this from the examples README / including it in the cargo.toml. Unsure if it's the right choice!

@anchpop
Copy link
Contributor Author

anchpop commented Sep 10, 2022

@mockersf I want your opinion on the organization here. I think it's interesting that we're linking to this from the examples README / including it in the cargo.toml. Unsure if it's the right choice!

One problem with this structure is that it causes cargo to print Warning: file tests/automated_tests.rs is included in multiple build targets (or something along those lines, I'm working from memory). But CI fails I include it in the examples page without including it in Cargo.toml, because running cargo run -p build-example-pages fails (as it detects there is something in the examples page that it thinks should not be there).

@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch 2 times, most recently from ee830be to 21fbed7 Compare September 10, 2022 19:59
@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch from 21fbed7 to 075ec8e Compare September 10, 2022 20:34
@anchpop anchpop force-pushed the @anchpop/add-test-plugins branch from 075ec8e to 200587b Compare September 10, 2022 20:44
@Shatur
Copy link
Contributor

Shatur commented Sep 17, 2022

When I test something on CI I add only plugins I need. So I wouldn't add a specific plugin group.

@alice-i-cecile
Copy link
Member

Closing out; I don't think this is a reliable enough solution to include at the engine level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostics Logging, crash handling, error reporting and performance analysis C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants