Skip to content

Conversation

@MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Aug 19, 2024

Objective

It looks like running compile_fail_utils::test on an invalid path just skips the test. I'm not sure why ui_test doesn't fail the test, but it seems pretty easy to accidentally cause a test to be skipped (I experienced this while doing some refactoring on bevy_reflect).

Solution

Check to make sure the given path exists before continuing on with the tests.

Alternatively, we could look into seeing why this doesn't work properly upstream. But I figured this solution was simple enough just to implement directly without having to worry about updating ui_test.

Testing

To verify that this works as expected cd into crates/bevy_reflect/compile_fail. Then run the following:

cargo test --target-dir ../../../target

All compile fail tests should pass. Now edit the path used in crates/bevy_reflect/compile_fail/tests/derive.rs. For example:

fn main() -> compile_fail_utils::ui_test::Result<()> {
-    compile_fail_utils::test("reflect_derive", "tests/reflect_derive")
+    compile_fail_utils::test("reflect_derive", "tests/does_not_exist")
}

Run the tests again:

cargo test --target-dir ../../../target

Verify the test fails with an error like:

Error: path does not exist: "tests/does_not_exist"

@MrGVSV MrGVSV added A-Build-System Related to build systems or continuous integration C-Testing A change that impacts how we test Bevy or how users test their apps D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 19, 2024
Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

LGTM

@TrialDragon TrialDragon 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 Aug 20, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 22, 2024
Merged via the queue into bevyengine:main with commit f8ef767 Aug 22, 2024
@MrGVSV MrGVSV deleted the mrgvsv/tools/compile-fail-path-exists branch August 22, 2024 22:49
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-Testing A change that impacts how we test Bevy or how users test their apps 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants