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

Refactor ci_testing and separate it from DevToolsPlugin #13513

Merged
merged 9 commits into from
May 26, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented May 26, 2024

Objective

Solution

  • Convert ci_testing.rs into the folder ci_testing, splitting the configuration and systems into ci_testing/config.rs and ci_testing/systems.rs.
  • Convert setup_app into the plugin CiTestingPlugin. This new plugin is added to both DefaultPlugins and MinimalPlugins.
  • Remove DevToolsPlugin from MinimalPlugins, since it was only used for CI testing.
  • Clean up some code, add many comments, and add a few unit tests.

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:

# Run the breakout example, enabling `bevy_ci_testing` and loading the configuration used in CI.
CI_TESTING_CONFIG=".github/example-run/breakout.ron" cargo r --example breakout -F bevy_ci_testing

Changelog

  • Added CiTestingPlugin, which is split off from DevToolsPlugin.
  • Removed DevToolsPlugin from MinimalPlugins.

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 in MinimalPlugins, so you will need to remove it manually.

// Before
App::new()
    .add_plugins(MinimalPlugins)
    .run();

// After
App::new()
    .add_plugins(MinimalPlugins)
    .add_plugins(DevToolsPlugin)
    .run();

@BD103 BD103 requested a review from mockersf May 26, 2024 04:34
@BD103 BD103 added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change C-Testing A change that impacts how we test Bevy or how users test their apps M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Domain-Expert Requires deep knowledge in a given domain S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 26, 2024
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.

Much better: I like this organization. This definitely shouldn't be part of DevToolsPlugin, and DevToolsPlugin should not be part of MinimalPlugins.

Copy link
Contributor

@pablo-lua pablo-lua left a 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

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 26, 2024
Copy link
Member

@matiqo15 matiqo15 left a 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!

@BD103
Copy link
Member Author

BD103 commented May 26, 2024

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 dev_tools is as good a place as any.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 26, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 26, 2024
Co-authored-by: François Mockers <[email protected]>
@alice-i-cecile alice-i-cecile enabled auto-merge May 26, 2024 22:17
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 26, 2024
Merged via the queue into bevyengine:main with commit b0409f6 May 26, 2024
27 checks passed
@BD103 BD103 deleted the refactor-ci-testing branch May 26, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change C-Testing A change that impacts how we test Bevy or how users test their apps D-Domain-Expert Requires deep knowledge in a given domain D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants