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 UI tests for existing lints #128

Merged
merged 10 commits into from
Oct 13, 2024
Merged

Add UI tests for existing lints #128

merged 10 commits into from
Oct 13, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Oct 8, 2024

Part of #31.

This is a continuation of #125 that actually adds UI tests for all of the current lints. It was split off to make #125 easier to review.

To test this, run:

cargo test -p bevy_lint --test ui

To bless changes, run:

cargo test -p bevy_lint --test ui -- --bless

There are a few additional options available if you replace --bless with --help, too.

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Testing A change to the tests S-Blocked This cannot move forward until something else changes labels Oct 8, 2024
@BD103 BD103 marked this pull request as ready for review October 9, 2024 02:41
@BD103
Copy link
Member Author

BD103 commented Oct 9, 2024

You'll probably notice the large file count when reviewing this PR. This is due to a multitude of reasons:

  1. Seeing a wall of green checkmarks brings me happiness.
  2. I like testing every single aspect of my code, even the bugs, to ensure I always know when something changes.
  3. Certain tests are required to be split into multiple files, such as main_return_without_appexit where you cannot have multiple fn main() in the same file. (Other examples include //@check-pass comments, which ensure no warnings are emitted for a file.)
  4. For each test that emits warnings / errors, a .stderr file is emitted with the plain-text content of the warning. If these diagnostics contain machine-applicable fixes, a .fixed file is also emitted by rustfix.

If you ever have trouble understanding a test, it's usually my fault and not yours. Please let me know if that's the case! You can also consult ui_test's README and examples.

@BD103
Copy link
Member Author

BD103 commented Oct 9, 2024

With that all out of the way, I've finished writing tests for all of our lints! This is ready for review, once #125 is merged. :)

@BD103 BD103 linked an issue Oct 10, 2024 that may be closed by this pull request
@BD103 BD103 mentioned this pull request Oct 12, 2024
@BD103 BD103 self-assigned this Oct 12, 2024
@BD103 BD103 removed the S-Blocked This cannot move forward until something else changes label Oct 12, 2024
@BD103
Copy link
Member Author

BD103 commented Oct 12, 2024

#125 is merged, so this is ready for review now!

Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Really nice to have tests for these.

Left some minor comments, but feel free to merge once you think it's ready.

bevy_lint/tests/ui/insert_event_resource/main.fixed Outdated Show resolved Hide resolved
bevy_lint/tests/ui/panicking_methods/query.rs Outdated Show resolved Hide resolved
bevy_lint/tests/ui/panicking_methods/query_state.rs Outdated Show resolved Hide resolved
bevy_lint/tests/ui/panicking_methods/world.rs Outdated Show resolved Hide resolved
@BD103 BD103 enabled auto-merge (squash) October 13, 2024 16:24
@BD103 BD103 merged commit 603a7c6 into main Oct 13, 2024
7 checks passed
@BD103 BD103 deleted the create-ui-tests branch October 13, 2024 16:33
@BD103 BD103 removed their assignment Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Related to the linter and custom lints C-Testing A change to the tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup UI tests for lints
2 participants