Skip to content

Migrate away from ad-hoc snapshot testing for rustfix test suite #13891

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
Closed
weihanglo opened this issue May 9, 2024 · 4 comments · Fixed by #15429
Closed

Migrate away from ad-hoc snapshot testing for rustfix test suite #13891

weihanglo opened this issue May 9, 2024 · 4 comments · Fixed by #15429
Assignees
@yatinmaan
Labels
A-testing-cargo-itself Area: cargo's tests C-cleanup Category: cleanup within the codebase Command-fix E-easy Experience: Easy P-low Priority: Low S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review

Comments

@weihanglo
Copy link
Member

weihanglo commented May 9, 2024 •
edited by rustbot
Loading

Problem

When dealing with rustfix snapshot test fixes, I found it a bit cumbersome to figure out the actual failure. You need to pass RUST_LOG=parse_and_replace=info to see the log, and RUSTFIX_TEST_BLESS=test-name.rs to update snapshots.

Proposed Solution

In the main Cargo crate, we've already integrated snapbox for UI tests. We could migrate rustfix test suite to that, and maybe share some common infra, like nightly channel detection from cargo-test-macro.

Notes

ehuss has a proposal that we don't run rustc if JSON snapshot exists: #13890 (comment).
This could be a good enhancement, and I think we can leave it to follow-ups.
@weihanglo weihanglo added C-cleanup Category: cleanup within the codebase A-testing-cargo-itself Area: cargo's tests S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review Command-fix E-easy Experience: Easy P-low Priority: Low labels May 9, 2024
@yatinmaan
Copy link

@rustbot claim

@epage
Copy link
Contributor

epage commented Jul 22, 2024

FYI I had looked into this in the past and it was a bit messy because we only care about updating the rustc-output snapshots if the rustfix snapshots fail.

@Pyr0de
Copy link
Contributor

Pyr0de commented Apr 11, 2025

Hi,
I'd like to like to try to solve this issue if @yatinmaan is not working on it.

Are all the RUSTFIX_TEST_* environment variables to be replaced by SNAPSHOTS=overwrite?
If so then on what conditions should the .json or .fixed.rs file be updated?

When SNAPSHOTS=overwrite,
If the json file is different from the rustc-output, then update the json file
If the applied changes are different, then update the fixed.rs file

@yatinmaan
Copy link

@Pyr0de sure, go ahead

@Pyr0de Pyr0de mentioned this issue Apr 13, 2025
Merged
github-merge-queue bot pushed a commit that referenced this issue Apr 14, 2025
@weihanglo
test(rustfix): Use snapbox for snapshot testing (#15429)
db99ddc 
### What does this PR try to resolve?

- separates each test into different test cases
- `snapbox` is used to test the snapshots
  - difference in `.json` file alone should never cause a test to fail
- `.json` files updated only if expected fix != actual fix &&
`SNAPSHOTS=overwrite`
- when `.json` files are updated, the json is pretty printed
- replaced environment variables `RUSTFIX_TEST_*` for overwriting test
snapshots with `SNAPSHOTS=overwrite`
- ❗ The `RUSTFIX_TEST_RECORD_FIXED_RUST` feature is removed (generate a
`*.fixed.rs` on demand`). We can add it back whenever needed.

Fixes #13891

### How should we test and review this PR?

Run tests with:
```sh
cargo test -p rustfix
```
All the test should run as different test cases
nightly tests run only when using nightly version of rustc is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yatinmaan yatinmaan

Labels
A-testing-cargo-itself Area: cargo's tests C-cleanup Category: cleanup within the codebase Command-fix E-easy Experience: Easy P-low Priority: Low S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test(rustfix): Use snapbox for snapshot testing Pyr0de/cargo
4 participants
@epage @yatinmaan @weihanglo @Pyr0de