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

Move compile fail tests #13196

Merged
merged 8 commits into from
May 3, 2024
Merged

Conversation

BD103
Copy link
Member

@BD103 BD103 commented May 3, 2024

Objective

  • Follow-up of Add README.md to all crates #13184 :)
  • We use ui_test to test compiler errors for our custom macros.
  • There are four crates related to compile fail tests
    • bevy_ecs_compile_fail_tests, bevy_macros_compile_fail_tests, and bevy_reflect_compile_fail_tests, which actually test the macros.
    • bevy_compile_test_utils, which provides helpers and common patterns for these tests.
  • All of these crates reside within the crates directory.
  • This can be confusing, especially for newcomers. All of the other folders in crates are actual published libraries, except for these 4.

Solution

  • Move all compile fail tests to a compile_fail folder under their corresponding crate.
    • E.g. crates/bevy_ecs_compile_fail_tests would be moved to crates/bevy_ecs/compile_fail.
  • Move bevy_compile_test_utils to tools/compile_fail_utils.

There are a few benefits to this approach:

  1. An internal testing detail is less intrusive (and confusing) for those who just want to browse the public Bevy interface.
  2. Follows a pre-existing approach of organizing related crates inside a larger crate's folder.
    • See bevy_gizmos/macros for an example.
  3. Makes consistent the terms compile_test, compile_fail, and compile_fail_test in code. It's all just compile_fail now, because we are specifically testing the error messages on compiler failures.
    • To be clear it can still be referred to by these terms in comments and speech, just the names of the crates and the CI command are now consistent.

Testing

Run the compile fail CI command:

cargo run -p ci -- compile-fail

If it still passes, then my refactor was successful.

BD103 added 7 commits May 2, 2024 22:20
When I think of the `crates` folder, I think of all of Bevy's modules. I don't expect an internal utility crate that is not even on crates.io.

While `tools` may not be the best location for it, I think its better than `crates`.
This is a completely opinionated change. (Though one could argue this entire PR is opinionated too.) I'd be willing to revert this if anyone disagrees.
@BD103 BD103 added A-ECS Entities, components, systems, and events A-Build-System Related to build systems or continuous integration A-Reflection Runtime information about types labels May 3, 2024
@BD103
Copy link
Member Author

BD103 commented May 3, 2024

cc @Brezak, who previously worked on migrating our compile fail tests to ui_test in #12810.

@alice-i-cecile alice-i-cecile self-requested a review May 3, 2024 03:04
@BD103
Copy link
Member Author

BD103 commented May 3, 2024

The files changed (105) may be intimidating, but it's not actually that bad. The actual lines changed are just +41 -39, the rest are just moved, unmodified files.

image

Copy link
Contributor

@Brezak Brezak left a comment

Choose a reason for hiding this comment

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

Looks ok. The documentation just needs to be fixed

crates/bevy_derive/compile_fail/README.md Show resolved Hide resolved
crates/bevy_ecs/compile_fail/README.md Show resolved Hide resolved
crates/bevy_reflect/compile_fail/README.md Show resolved Hide resolved
tools/compile_fail_utils/README.md Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples C-Code-Quality A section of code that is hard to understand or change and removed C-Usability A simple quality-of-life change that makes Bevy easier to use labels May 3, 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.

I agree with the changes here: these are way less confusing if they're not top-level crates.

Once the links are fixed this LGTM.

@alice-i-cecile alice-i-cecile added D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills X-Uncontroversial This work is generally agreed upon labels May 3, 2024
Copy link
Contributor

@soqb soqb left a comment

Choose a reason for hiding this comment

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

LGTM.

side note: i feel like we shouldn't put compile-succeed tests in these crates, since its much easier iteratively to drop them into regular #[test]s, since these tests are still flaky outside of ci.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 3, 2024
Merged via the queue into bevyengine:main with commit bdb4899 May 3, 2024
34 checks passed
@BD103 BD103 deleted the move-compile-fails branch May 3, 2024 14:44
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 A-ECS Entities, components, systems, and events A-Reflection Runtime information about types C-Code-Quality A section of code that is hard to understand or change D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants