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.

Sign up for GitHub

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

Jump to bottom

Implement UI testing framework #125

Merged
merged 22 commits into from
Oct 12, 2024
Merged

Implement UI testing framework #125

merged 22 commits into from
Oct 12, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Oct 4, 2024

Closes #31. Previously attempted in #32. Rebased off of #124, so blocked until that is merged.

This PR implements UI tests: special programs that check lint diagnostics. UI tests can be used to ensure that a lint functions correctly, and handles all edge cases.

This has been a long time in the making, but I'm so happy that it finally works! :D

@BD103 BD103 added A-Linter Related to the linter and custom lints C-Testing A change to the tests labels Oct 4, 2024
@BD103 BD103 marked this pull request as ready for review October 4, 2024 13:21
@BD103
Copy link
Member Author

BD103 commented Oct 4, 2024

CI is failing due to multiple .rlib / .rmeta files being found for bevy. This is due to caching bringing in old compilations of Bevy, and rustc being unsure which to choose. Cargo usually figures this out for us, but the UI tests use rustc directly, which cannot guess.

In the past I fixed this by calling cargo clean and rebuilding everything from scratch, but tests already take long enough, so I'm going to see if I can somehow specify the exact .rmeta file that tests should link to.

Looks like rustc_metadata::locator is promising. I'll try reading that!

@BD103
Copy link
Member Author

BD103 commented Oct 5, 2024

I opened a thread in the Rust Zulip, so hopefully someone gets back to me on how to solve this issue. I fear I may have to either add cargo as a dependency or remove caching completely for tests, so I'm hoping there's a better solution that I didn't think of.

@BD103 BD103 added S-Blocked This cannot move forward until something else changes and removed S-Blocked This cannot move forward until something else changes labels Oct 5, 2024
@BD103 BD103 marked this pull request as draft October 6, 2024 20:53
This was referenced Oct 7, 2024
Open
Closed
@BD103 BD103 marked this pull request as ready for review October 8, 2024 00:53
@BD103
Copy link
Member Author

BD103 commented Oct 8, 2024

I'm going to split off the actual UI tests into a separate PR, to make review of this one a bit more digestible. The one UI test that I did add is a proof of concept, and will probably be modified when the rest of the tests are created.

@BD103 BD103 changed the title Implement UI tests Implement UI testing framework Oct 8, 2024
@BD103 BD103 mentioned this pull request Oct 8, 2024
Merged
TimJentzsch
TimJentzsch reviewed Oct 12, 2024
View reviewed changes
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.

Exciting stuff! I have a few clarifying questions, but it looks good already

bevy_lint/tests/ui.rs Outdated Show resolved Hide resolved
bevy_lint/tests/ui.rs Outdated Show resolved Hide resolved
BD103 added 2 commits October 12, 2024 10:41
@BD103
fix: parse stdout as UTF-8
84017df
@BD103
fix: do not check package ID when searching for bevy .rlib
6dd0042 
Also add `#[serde(borrow)]` annotations, since they were missing.
@BD103
Copy link
Member Author

BD103 commented Oct 12, 2024

One unfortunate part of UI tests is that they aren't automatically formatted by cargo fmt. I may be able to add something to CI to check for that, but that will probably be in a follow-up.

@BD103 BD103 mentioned this pull request Oct 12, 2024
Closed
@BD103
Copy link
Member Author

BD103 commented Oct 12, 2024

Thanks for the review! :)

@BD103 BD103 merged commit f969f96 into main Oct 12, 2024
7 checks passed
@BD103 BD103 deleted the ui-tests-v2 branch October 12, 2024 20:51
BD103 added a commit that referenced this pull request Oct 13, 2024
@BD103
Add UI tests for existing lints (#128)
603a7c6 
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:

```bash
cargo test -p bevy_lint --test ui
```

To bless changes, run:

```bash
cargo test -p bevy_lint --test ui -- --bless
```

There are a few additional options available if you replace `--bless`
with `--help`, too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimJentzsch TimJentzsch TimJentzsch approved these changes

Assignees
No one assigned
Labels
A-Linter Related to the linter and custom lints C-Testing A change to the tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Setup UI tests for lints
2 participants
@BD103 @TimJentzsch