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

Refactor cargo lint tests #13880

Merged
merged 3 commits into from
May 9, 2024
Merged

Conversation

Muscraft
Copy link
Member

@Muscraft Muscraft commented May 8, 2024

In #13621, it was brought up that the lints tests are nested more deeply than other UI tests. This got me wondering if there was a better way to structure all the lint tests.
What I came up with was:

  • Lints should not have UI tests, only parts of the diagnostic system, i.e., how warnings, errors, notes, etc., look
    • This is because UI tests should focus on parts of the system that make up each lint's output
    • We can always add UI tests for each lint if desired
  • All tests related to the linting system should live in tests/testsuite/lints/
  • Tests related to [lints.cargo] should stay in lints_table.rs as it is for the whole lints table
  • Each lint will get a file in lints/ for all of its tests
    • This makes lints/mod.rs smaller and targeted only at tests for the linting system itself
    • It makes it much easier to know where to place a test

@rustbot
Copy link
Collaborator

rustbot commented May 8, 2024

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2024
.run();
}

// LINT_SPECIFIC_TESTS
Copy link
Member

Choose a reason for hiding this comment

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

I am fine with have cargo_lints.rs for organizing every lint test, though I don't understand why we can't have nested UI tests. I believe the amount of lint specific tests will grow beyond controls like testsuite/build.rs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nesting tests more deeply would not be a problem; the problem, from my understanding, is that they would diverge from how other UI tests are structured.

@Muscraft Muscraft force-pushed the refactor-cargo-lint-tests branch from a3015ec to b79fd59 Compare May 9, 2024 04:43
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good to me, and looking forward to documenting the lint development process in the future :)

Something is failing rustfix tests on nightly and I am working on it:


failures:

---- everything stdout ----
failed: ./tests/everything/multiple-solutions.rs
thread 'everything' panicked at crates/rustfix/tests/parse_and_replace.rs:233:9:
1 out of 9 fixture asserts failed
error: test failed, to rerun pass `-p rustfix --test parse_and_replace`
(run with `env RUST_LOG=parse_and_replace=info` to get more details)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@weihanglo
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented May 9, 2024

⌛ Trying commit b79fd59 with merge 0215181...

bors added a commit that referenced this pull request May 9, 2024
Refactor cargo lint tests

In #13621, it was brought up that [the lints tests are nested more deeply than other UI tests](#13621 (comment)). This got me wondering if there was a better way to structure all the lint tests.
What I came up with was:
- Lints should not have UI tests, only parts of the diagnostic system, i.e., how warnings, errors, notes, etc., look
  - This is because UI tests should focus on parts of the system that make up each lint's output
  - We can always add UI tests for each lint if desired
- All tests related to the linting system should live in `tests/testsuite/lints/`
- Tests related to `[lints.cargo]` should stay in `lints_table.rs` as it is for the whole lints table
- Each lint will get a file in `lints/` for all of its tests
  - This makes `lints/mod.rs` smaller and targeted only at tests for the linting system itself
  - It makes it much easier to know where to place a test
@bors
Copy link
Contributor

bors commented May 9, 2024

☀️ Try build successful - checks-actions
Build commit: 0215181 (0215181b1551503cb1509c0493055b13a5a9ab87)

@weihanglo
Copy link
Member

@bors r+

Thanks for the refactor!

@bors
Copy link
Contributor

bors commented May 9, 2024

📌 Commit b79fd59 has been approved by weihanglo

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2024
@bors
Copy link
Contributor

bors commented May 9, 2024

⌛ Testing commit b79fd59 with merge baca68e...

@bors
Copy link
Contributor

bors commented May 9, 2024

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing baca68e to master...

@bors bors merged commit baca68e into rust-lang:master May 9, 2024
13 of 21 checks passed
@Muscraft Muscraft deleted the refactor-cargo-lint-tests branch May 9, 2024 16:25
bors added a commit to rust-lang-ci/rust that referenced this pull request May 11, 2024
Update cargo

7 commits in 0ca60e940821c311c9b25a6423b59ccdbcea218f..4de0094ac78743d2c8ff682489e35c8a7cafe8e4
2024-05-08 01:54:25 +0000 to 2024-05-09 16:09:22 +0000
- Fix docs for unstable script feature (rust-lang/cargo#13893)
- Refactor cargo lint tests (rust-lang/cargo#13880)
- test(rustfix): run some tests only on nightly (rust-lang/cargo#13890)
- Old syntax suggestion (rust-lang/cargo#13874)
- docs: clarify dash replacement rule in target name (rust-lang/cargo#13887)
- Add local-only build scripts example in check-cfg docs (rust-lang/cargo#13884)
- docs(changelog): also mention `--message-format=json` (rust-lang/cargo#13882)

r? ghost
@rustbot rustbot added this to the 1.80.0 milestone May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants