Check `.fixed` paths' existence in `run_ui` #8844

smoelius · 2022-05-19T16:37:57Z

This PR adds a test to check that there exists a .fixed file for every .stderr file in tests/ui that mentions a MachineApplicable lint. The test leverages compiletest-rs's rustfix_coverage option.

I tried to add .fixed files where they appeared to be missing. However, 38 exceptional .rs files remain. Several of those include comments indicating that they are exceptions, though not all do. Apologies, as I have not tried to associate the 38 files with GH issues. (I think that would be a lot of work, and I worry about linking the wrong issue.)

changelog: none

rust-highfive · 2022-05-19T16:38:00Z

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

xFrednet · 2022-05-19T16:45:12Z

I believe there are a few lints with different lint emissions and applicability based on the context. 🙃 If we add something like this, I think we should add some option to disable it for specific test files.

cc: @rust-lang/clippy, @Alexendoo, @dswij, @Jarcho

smoelius · 2022-05-19T16:50:25Z

If we add something like this, I think we should add some option to disable it for specific test files.

I'm not sure if this is what you have in mind, but that sounds like what I did here: https://github.com/rust-lang/rust-clippy/pull/8844/files#diff-30869434167a8e693c01b4875d4855efeb3a28567f6531765b202e82cc398a99R59

tests/dogfood.rs

xFrednet · 2022-05-21T14:34:09Z

It looks like nobody has any concerns regarding this change, and I also like it.

r? @Alexendoo

Alexendoo · 2022-05-21T17:16:24Z

I like the idea, trying it locally it does take quite a while to run though, took up to a minute for me. compiletest has rustfix_coverage for this that we could enable in run_ui, it creates a text file with the list of relevant files

Unfortunately the version of rustfix in the fork crashes when this is enabled, it's fixed upstream (rust-lang/rustfix#199) but I don't know what the process for updating the compiletest fork would be (sync all the upstream changes or just bump the rustfix version?)

smoelius · 2022-05-21T22:30:31Z

I don't know what the process for updating the compiletest fork would be (sync all the upstream changes or just bump the rustfix version?)

Sorry, but I don't understand what the two alternatives are (possibly because I don't know what "the compiletest fork" refers to).

One alternative is to bump the rustfix version in compiletest, and once compiletest is updated, (possibly) bump the compiletest version in Ciippy (correct?).

What is the other alternative?

Alexendoo · 2022-05-21T23:05:39Z

Clippy uses https://github.com/Manishearth/compiletest-rs, which is extracted from https://github.com/rust-lang/rust/tree/master/src/tools/compiletest. The one in rust-lang/rust has the updated rustfix dep of 0.6.0, but I wasn't sure what the usual way to get upstream changes into the compiletest-rs repo is

The sync all thing would be to round up all of the upstream changes in one PR, but looking at other PRs to the repo I don't think that's how it's done. So a PR that bumps the rustfix version alone should be good

smoelius · 2022-05-22T10:59:52Z

Submitted. I'll update this PR once a new version of compiletest-rs is published.

smoelius · 2022-05-22T23:41:34Z

@Alexendoo I'm sorry if you were telling me this and I didn't get it: rust-lang/rustfix#199 is not in rustfix 0.6.0. So, I've requested a new release of rustfix.

Alexendoo · 2022-05-23T10:26:32Z

Oh whoops I thought it was, my mistake

smoelius · 2022-05-24T09:34:41Z

I've updated the PR to use rustfix_coverage. You're welcome to review whenever you have time.

Alexendoo

Looks great thanks!

You can also now merge these into their respective main test files, or delete them if the contents are duplicates

tests/ui/needless_late_init_fixable.rs
tests/ui/unnecessary_cast_fixable.rs

tests/compile-test.rs

smoelius · 2022-05-25T00:02:09Z

@Alexendoo I think I have addressed all of your comments.

tests/compile-test.rs

Alexendoo

Thanks @smoelius!

LGTM @xFrednet

xFrednet · 2022-05-27T16:59:14Z

Looks good to me, thank you for this addition :)

Also thank you, @Alexendoo, for the nice suggestion with the updated dependency. I wouldn't have known about that!

@bors r=Alexendoo,xFrednet

bors · 2022-05-27T16:59:16Z

📌 Commit 534fc5d has been approved by Alexendoo,xFrednet

bors · 2022-05-27T16:59:22Z

⌛ Testing commit 534fc5d with merge 82c52e8...

Check `.fixed` paths' existence in `run_ui` This PR adds a test to check that there exists a `.fixed` file for every `.stderr` file in `tests/ui` that mentions a `MachineApplicable` lint. The test leverages `compiletest-rs`'s `rustfix_coverage` option. I tried to add `.fixed` files where they appeared to be missing. However, 38 exceptional `.rs` files remain. Several of those include comments indicating that they are exceptions, though not all do. Apologies, as I have not tried to associate the 38 files with GH issues. (I think that would be a lot of work, and I worry about linking the wrong issue.) changelog: none

bors · 2022-05-27T17:24:45Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

bors · 2022-05-27T17:24:45Z

📌 Commit 534fc5d has been approved by xFrednet

bors · 2022-05-27T17:24:52Z

⌛ Testing commit 534fc5d with merge 49aa6c5...

Check `.fixed` paths' existence in `run_ui` This PR adds a test to check that there exists a `.fixed` file for every `.stderr` file in `tests/ui` that mentions a `MachineApplicable` lint. The test leverages `compiletest-rs`'s `rustfix_coverage` option. I tried to add `.fixed` files where they appeared to be missing. However, 38 exceptional `.rs` files remain. Several of those include comments indicating that they are exceptions, though not all do. Apologies, as I have not tried to associate the 38 files with GH issues. (I think that would be a lot of work, and I worry about linking the wrong issue.) changelog: none

xFrednet · 2022-05-27T17:25:16Z

@bors r-

xFrednet · 2022-05-27T17:25:56Z

@bors r=Alexendoo,xFrednet

bors · 2022-05-27T17:25:58Z

📌 Commit 534fc5d has been approved by Alexendoo,xFrednet

Check `.fixed` paths' existence in `run_ui` This PR adds a test to check that there exists a `.fixed` file for every `.stderr` file in `tests/ui` that mentions a `MachineApplicable` lint. The test leverages `compiletest-rs`'s `rustfix_coverage` option. I tried to add `.fixed` files where they appeared to be missing. However, 38 exceptional `.rs` files remain. Several of those include comments indicating that they are exceptions, though not all do. Apologies, as I have not tried to associate the 38 files with GH issues. (I think that would be a lot of work, and I worry about linking the wrong issue.) changelog: none

bors · 2022-05-27T17:26:04Z

⌛ Testing commit 534fc5d with merge 54f237f...

bors · 2022-05-27T17:37:53Z

💔 Test failed - checks-action_test

Alexendoo · 2022-05-27T17:46:22Z

👀 Oh whoops that's my fault #8896

Maybe it would be worth ignoring the crashes dir though

smoelius · 2022-05-27T17:48:40Z

👀 Oh whoops that's my fault #8896

Maybe it would be worth ignoring the crashes dir though

In a way, this is a success! :D

What is your preference? I rebase and fix this, or ignore the the crashes dir (and maybe revert the changes thereto)?

smoelius · 2022-05-27T19:27:23Z

I rebased and added a commit for ice-8850. Please let me know if you would rather ignore the crashes dir.

xFrednet · 2022-05-27T19:31:34Z

I think it would be good to ignore the crashes directory. The focus of those UI tests is not to test the lint, but the lint implementation itself. Either option would be correct, though. If it's simple to do, I would appreciate that change. 🙃

smoelius · 2022-05-27T20:42:37Z

Oops, I forgot about Windows again.

xFrednet · 2022-05-27T20:45:52Z

This version looks good to me, thank you for the work! I'll leave the final r+ to @Alexendoo 🙃

@bors delegate=Alexendoo

bors · 2022-05-27T20:45:54Z

✌️ @Alexendoo can now approve this pull request

Alexendoo · 2022-05-28T11:27:59Z

Looks all good to me too, thanks @smoelius

@bors r+

bors · 2022-05-28T11:28:00Z

📌 Commit 6027255 has been approved by Alexendoo

bors · 2022-05-28T11:28:06Z

⌛ Testing commit 6027255 with merge 5920fa3...

bors · 2022-05-28T11:43:42Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 5920fa3 to master...

rust-highfive assigned flip1995 May 19, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 19, 2022

smoelius force-pushed the fixed-paths branch from ae6ab06 to 3e92ded Compare May 19, 2022 16:46

xFrednet reviewed May 19, 2022

View reviewed changes

tests/dogfood.rs Outdated Show resolved Hide resolved

xFrednet self-assigned this May 21, 2022

smoelius mentioned this pull request May 22, 2022

Update rustfix dependency Manishearth/compiletest-rs#252

Closed

smoelius mentioned this pull request May 22, 2022

New release? rust-lang/rustfix#211

Closed

smoelius force-pushed the fixed-paths branch from 734dabf to 9968398 Compare May 24, 2022 09:25

smoelius changed the title ~~Add fixed_path_existence test~~ Check .fixed paths' existence in run_ui May 24, 2022

Alexendoo reviewed May 24, 2022

View reviewed changes

tests/compile-test.rs Outdated Show resolved Hide resolved

tests/compile-test.rs Outdated Show resolved Hide resolved

tests/compile-test.rs Show resolved Hide resolved

xFrednet assigned Alexendoo May 25, 2022

Alexendoo reviewed May 25, 2022

View reviewed changes

tests/compile-test.rs Outdated Show resolved Hide resolved

Alexendoo approved these changes May 25, 2022

View reviewed changes

xFrednet approved these changes May 27, 2022

View reviewed changes

smoelius added 3 commits May 27, 2022 15:18

Check .fixed paths' existence in run_ui

911eb1f

Address review comments

c91a7f0

Fix assert! message

8f49e41

smoelius force-pushed the fixed-paths branch from 534fc5d to 25b4f1f Compare May 27, 2022 19:25

smoelius force-pushed the fixed-paths branch from 25b4f1f to 309a7f6 Compare May 27, 2022 20:39

Ignore crashes dir

6027255

smoelius force-pushed the fixed-paths branch from 309a7f6 to 6027255 Compare May 27, 2022 20:44

bors merged commit 5920fa3 into rust-lang:master May 28, 2022

smoelius deleted the fixed-paths branch May 28, 2022 12:04

Check .fixed paths' existence in run_ui #8844

Check .fixed paths' existence in run_ui #8844

Conversation

smoelius commented May 19, 2022 • edited Loading

rust-highfive commented May 19, 2022

xFrednet commented May 19, 2022 • edited Loading

smoelius commented May 19, 2022

xFrednet commented May 21, 2022

Alexendoo commented May 21, 2022

smoelius commented May 21, 2022

Alexendoo commented May 21, 2022

smoelius commented May 22, 2022

smoelius commented May 22, 2022

Alexendoo commented May 23, 2022

smoelius commented May 24, 2022

Alexendoo left a comment

Choose a reason for hiding this comment

smoelius commented May 25, 2022

Alexendoo left a comment

Choose a reason for hiding this comment

xFrednet commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

xFrednet commented May 27, 2022

xFrednet commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

bors commented May 27, 2022

Alexendoo commented May 27, 2022

smoelius commented May 27, 2022 • edited Loading

smoelius commented May 27, 2022 • edited Loading

xFrednet commented May 27, 2022

smoelius commented May 27, 2022

xFrednet commented May 27, 2022 • edited Loading

bors commented May 27, 2022

Alexendoo commented May 28, 2022

bors commented May 28, 2022

bors commented May 28, 2022

bors commented May 28, 2022

Check `.fixed` paths' existence in `run_ui` #8844

Check `.fixed` paths' existence in `run_ui` #8844

smoelius commented May 19, 2022 •

edited

Loading

xFrednet commented May 19, 2022 •

edited

Loading

smoelius commented May 27, 2022 •

edited

Loading

smoelius commented May 27, 2022 •

edited

Loading

xFrednet commented May 27, 2022 •

edited

Loading