-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Refactor ci_testing
and separate it from DevToolsPlugin
#13513
Conversation
…instead This is used instead of adding it to `DevToolsPlugin`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better: I like this organization. This definitely shouldn't be part of DevToolsPlugin
, and DevToolsPlugin
should not be part of MinimalPlugins
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed here, Ci testing shouldn't be there, so all good! Though we should do something about the devtools plugin soon, but not anywhere part of this PR, LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether we want to keep the bevy_ci_testing
feature in the bevy_dev_tools
. Overall looks good!
If we were to split it out into a separate crate, I would like it to be a bit more general purpose. I don't think its polished enough for that yet, so for now |
Co-authored-by: François Mockers <[email protected]>
Objective
ci_testing
to specify per-example configuration on when to take a screenshot, when to exit, etc.ci_testing
#13512. To support this growth,ci_testing
should be easier to read and maintain.Solution
ci_testing.rs
into the folderci_testing
, splitting the configuration and systems intoci_testing/config.rs
andci_testing/systems.rs
.setup_app
into the pluginCiTestingPlugin
. This new plugin is added to bothDefaultPlugins
andMinimalPlugins
.DevToolsPlugin
fromMinimalPlugins
, since it was only used for CI testing.Testing
The most important part is that this still passes all of the CI validation checks (merge queue), since that is when it will be used the most. I don't think I changed any behavior, so it should operate the same.
You can also test it locally using:
Changelog
CiTestingPlugin
, which is split off fromDevToolsPlugin
.DevToolsPlugin
fromMinimalPlugins
.Migration Guide
Hi maintainers! I believe
DevToolsPlugin
was added within the same release as this PR, so I don't think a migration guide is needed.DevToolsPlugin
is no longer included inMinimalPlugins
, so you will need to remove it manually.