add tidy check that forbids issue-XXXX test filenames #113583

asquared31415 · 2023-07-11T18:45:11Z

Helps with #113345 by preventing any future tests with non-descriptive names from being added.

This PR only checks modified ui test files because there are far too many existing problematic tests to be fixed at once:
3063/15424 (~19.86%) *.rs ui test files match ^issue[-_ ]?\d+$.
Another 1349 files, totaling ~28.60% of all ui test files, contain that pattern in addition to some other text, where they should probably omit it in favor of a comment.

note: between the creation of this PR and 2023-07-25 (14 days), 10 more tests were added that failed this check.

r? @workingjubilee

rustbot · 2023-07-11T18:45:20Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

src/tools/tidy/src/ui_tests.rs

compiler-errors · 2023-07-11T18:49:12Z

src/tools/tidy/src/ui_tests.rs

+                if let Some(m) = regex.find(name) {
+                    tidy_error!(
+                        bad,
+                        "test file {} should not use {} in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",


Suggested change

"test file {} should not use {} in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",

"test file `{}` should not use `{}` in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",

Can this be slightly clearer that it isn't the issue number that is totally forbidden from the filename, but that the filename needs to actually identify what the test is about at-a-glance, i.e. what-it-tests-issue-\d+ is acceptable, strictly speaking (if the reviewer accepts it, at least)?

compiler-errors

lgtm

compiler-errors · 2023-07-11T18:50:06Z

src/tools/tidy/src/ui_tests.rs

@@ -128,4 +132,44 @@ pub fn check(path: &Path, bad: &mut bool) {
            }
        }
    });
+
+    // Check that any test files modified don't have a useless name
+    for changed_file in git::get_git_modified_files(None, &vec!["rs"]).unwrap().unwrap() {


hahah unrelated but why does get_git_modified_files take a &Vec<_> :thonk:

matthiaskrgr · 2023-07-11T19:03:35Z

Wondering if it would make sense to actually go the other way around; if a test file contains something like
https://github.com/rust-lang/rust/issues/113583 , then it MUST also contain "113583" in the filename? 🤔
(I'm a huge fan of having issue numbers in file names fwiw 😅 )

workingjubilee · 2023-07-11T19:11:14Z

That would break on the very likely case that a test resolves two issues and thus mentions two issues. No, I am not going to tolerate tests named issues-111111-122222-123333-123444-123455-123456-stack-overflow.rs.

Noratrieb · 2023-07-11T19:18:30Z

What purpose do numbers in filenames serve? @matthiaskrgr
I think it's way nicer to have them as a comment inside the test, it's just as searchable and doesn't clutter the name.

matthiaskrgr · 2023-07-11T19:18:35Z

Ooh right :D but we could special case that and make sure it contains one of them in the filename?

matthiaskrgr · 2023-07-11T19:31:56Z

@Nilstrieb when I am triaging my icemaker reports, I can see right away "issue-12345.rs" crashed with the -Zealous flag.
I then search #12345 in the issue tracker and if the results are "0 open, 1 closed" I already know that this is a new bug and can investigate further.

Its just much more useful to have the issue number right away to open the ticket and see the related discussion/status etc than having something only something like trait-as-impl-trait-ice-in-const-closure-context.rs That may or may not contain an actual link to the original issue inside.

Entering the code of the testcase in the github search does not always work because of minimization, and going through git log / git blame to find the a fixes 123 somewhere in a parent commit takes maybe 20-30 seconds which does not sound like much but it definitely adds up compares to 0.7 seconds of grabbing the issue number from the file name, especially if you try to triage 20-40 ices at a time.

Noratrieb · 2023-07-11T19:34:11Z

you could modify your code to try to extract issue numbers from the source, I think that short work in most cases too.

src/tools/tidy/src/ui_tests.rs

workingjubilee · 2023-07-11T19:38:22Z

src/tools/tidy/src/ui_tests.rs

+                if let Some(m) = regex.find(name) {
+                    tidy_error!(
+                        bad,
+                        "test file {} should not use {} in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",


Can this be slightly clearer that it isn't the issue number that is totally forbidden from the filename, but that the filename needs to actually identify what the test is about at-a-glance, i.e. what-it-tests-issue-\d+ is acceptable, strictly speaking (if the reviewer accepts it, at least)?

tgross35 · 2023-07-11T19:40:47Z

It would be nice to somehow enforce an attribute style so they're easily extractable, regardless of whether or not the issue number is in the title

// issue:12345
// issue:6789
// ice:1234

workingjubilee · 2023-07-11T20:01:26Z

I do not think we should enforce that issue numbers be in filenames for the benefit of automation. The directory structures are for humans, and machines can deal with skimming through raw inodes and bytes if they must. It is specifically because the issue numbers are effectively just a table of pointers to allocations of real data that we are discussing this lint.

matthiaskrgr · 2023-07-11T20:10:02Z

It is specifically because the issue numbers are effectively just a table of pointers to allocations of real data that we are

Exactly! and by having the issue number in the title/filename, you only need a single lookup to access the data 🙂
Hiding the pointer somewhere inside inside the actual file and having to actually open and read the file is just a ton of unnecessary overhead!

workingjubilee · 2023-07-11T20:15:03Z

And it's a ton of unnecessary overhead, when you are actually trying to examine the "deep history" of a bug, to have to open a bunch of files and examine each one for its purpose, rather than seeing the filename identifying things at a glance. And if a test relates to two or more issues, which is not at all uncommon now that we have over 50 thousand closed issues and over 8000 open issues, then having only one issue in the filename is equally useless if it's not the one you know the matter by.

@matthiaskrgr If you need me to PR code to you for extracting issue numbers from file contents, I am happy to put it on my todo list if you name the repo.

workingjubilee · 2023-07-11T20:16:42Z

@tgross35 Would you be so kind as to open an issue for that?

matthiaskrgr · 2023-07-11T20:21:47Z

Also note that it seems that the current consent is to actually prefer "issue-xyz in filename" over "issue number somewhere inside the file":

Looking just in tests/ui:

issuenr in file:
git ls-files | grep rs$ | grep "issue" | wc -l 4474

issuenr in comment:

workingjubilee · 2023-07-11T20:36:30Z

rg 'issue.*[0-9]{4,}' --files-without-match | rg 'rs$' | wc -l
17174

I rest my case.

matthiaskrgr · 2023-07-11T20:38:30Z

@workingjubilee oh maybe there is a bit of a misunderstanding, fwiw I have nothing against descriptive file names in addition to the issue number next to it, like rpitit-crash-on-unbound-whatever-103982.rs or issue-42167-OOM-on-recursive-thingamadoodle.rs but I really don't think we should rename all the files to never ever have a single digit in them "because there is already a link in the file itself" or something like that.

workingjubilee · 2023-07-11T20:41:46Z

That is why I asked to refine the regex to allow rpitit-crash-on-unbound-whatever-issue-103982.rs.

matthiaskrgr · 2023-07-11T20:52:10Z

Oh, and this will only affect ui tests, right? subrepo'd tools like clippy and rustfmt are still free to follow their own habits, right?

workingjubilee · 2023-07-11T20:56:28Z

If tidy is currently running on subtrees we already have a problem, please take that up in a new issue.

asquared31415 · 2023-07-11T21:48:47Z

@rustbot author
I need to figure out how to get this working in CI properly before addressing the other concerns about the exact things that are checked.

bors · 2023-07-26T02:50:52Z

💔 Test failed - checks-actions

workingjubilee · 2023-07-26T03:40:46Z

huh?

asquared31415 · 2023-07-26T03:41:45Z

💔 windows paths

(I'm doing string operations on hard coded paths)

workingjubilee · 2023-07-26T03:52:18Z

Oh, right.

workingjubilee · 2023-07-26T04:15:35Z

Sure, let's give this another go.

workingjubilee · 2023-07-26T04:16:06Z

@bors r+

bors · 2023-07-26T04:16:08Z

📌 Commit 13e2abf has been approved by workingjubilee

It is now in the queue for this repository.

bors · 2023-07-26T04:35:40Z

⌛ Testing commit 13e2abf with merge cf34adb...

matthiaskrgr · 2023-07-26T04:37:22Z

shouldn't this go through an mcp at least so that more than 5 people are aware of this?

compiler-errors · 2023-07-26T05:08:07Z

MCP was opened in rust-lang/compiler-team#658. I don't think this warrants stopping the merge, though.

bors · 2023-07-26T06:26:01Z

☀️ Test successful - checks-actions
Approved by: workingjubilee
Pushing cf34adb to master...

rust-timer · 2023-07-26T12:16:04Z

Finished benchmarking commit (cf34adb): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.3%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.5%, 2.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.7%	[-2.7%, -2.7%]	1
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 650.304s -> 650.76s (0.07%)

…t-name-lint, r=compiler-errors Reimpl meaningful test name lint MCP658 This reintroduces the tidy rule originally proposed in rust-lang#113583 that then became an MCP in rust-lang/compiler-team#658 which eventually surfaced a quite-reasonable request for a diagnostic enhancement. I have added that to the rule. It produces output like this: ``` tidy error: file `ui/unsized/issue-115809.rs` must begin with a descriptive name, try `{reason}-issue-115809.rs` tidy error: file `ui/unsized/issue-115203.rs` must begin with a descriptive name, try `{reason}-issue-115203.rs` tidy error: file `ui/privacy/issue-113860-2.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/issue-119463.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/auxiliary/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/auxiliary/issue-119463-extern.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/issue-113860-1.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-113860.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/const-generics/generic_const_exprs/const_kind_expr/issue_114151.rs` must begin with a descriptive name, try `{reason}-issue-114151.rs` tidy error: file `ui/did_you_mean/issue-114112.rs` must begin with a descriptive name, try `{reason}-issue-114112.rs` tidy error: file `ui/did_you_mean/issue-105225.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` tidy error: file `ui/did_you_mean/issue-105225-named-args.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` ``` You get the idea. There are some tests which merely would require reordering of the name according to the rule. I could modify the diagnostic further to identify those, but doing such would make it prone to bad suggestions. I have opted to trust contributors to recognize the diagnostic is robotic, as the pattern we are linting on is easier to match if we do not speculate on what parts of the name are meaningful: sometimes a word is a reason, but sometimes it is a mere "tag", such as with a pair like: - issue-314159265-blue.rs - issue-314159265-red.rs Starting them with `red-` and `blue-` means they do not sort together, despite being related, and the color names are still not very descriptive. Recognizing a good name is an open-ended task, though this pair might be: - colored-circle-gen-blue.rs - colored-circle-gen-red.rs Deciding exactly *how* to solve this is not the business of tidy, only recognizing a what. r? `@compiler-errors`

…nt, r=compiler-errors Reimpl meaningful test name lint MCP658 This reintroduces the tidy rule originally proposed in rust-lang/rust#113583 that then became an MCP in rust-lang/compiler-team#658 which eventually surfaced a quite-reasonable request for a diagnostic enhancement. I have added that to the rule. It produces output like this: ``` tidy error: file `ui/unsized/issue-115809.rs` must begin with a descriptive name, try `{reason}-issue-115809.rs` tidy error: file `ui/unsized/issue-115203.rs` must begin with a descriptive name, try `{reason}-issue-115203.rs` tidy error: file `ui/privacy/issue-113860-2.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/issue-119463.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/auxiliary/issue-117997.rs` must begin with a descriptive name, try `{reason}-issue-117997.rs` tidy error: file `ui/privacy/auxiliary/issue-119463-extern.rs` must begin with a descriptive name, try `{reason}-issue-119463.rs` tidy error: file `ui/privacy/issue-113860-1.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/privacy/issue-113860.rs` must begin with a descriptive name, try `{reason}-issue-113860.rs` tidy error: file `ui/const-generics/generic_const_exprs/const_kind_expr/issue_114151.rs` must begin with a descriptive name, try `{reason}-issue-114151.rs` tidy error: file `ui/did_you_mean/issue-114112.rs` must begin with a descriptive name, try `{reason}-issue-114112.rs` tidy error: file `ui/did_you_mean/issue-105225.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` tidy error: file `ui/did_you_mean/issue-105225-named-args.rs` must begin with a descriptive name, try `{reason}-issue-105225.rs` ``` You get the idea. There are some tests which merely would require reordering of the name according to the rule. I could modify the diagnostic further to identify those, but doing such would make it prone to bad suggestions. I have opted to trust contributors to recognize the diagnostic is robotic, as the pattern we are linting on is easier to match if we do not speculate on what parts of the name are meaningful: sometimes a word is a reason, but sometimes it is a mere "tag", such as with a pair like: - issue-314159265-blue.rs - issue-314159265-red.rs Starting them with `red-` and `blue-` means they do not sort together, despite being related, and the color names are still not very descriptive. Recognizing a good name is an open-ended task, though this pair might be: - colored-circle-gen-blue.rs - colored-circle-gen-red.rs Deciding exactly *how* to solve this is not the business of tidy, only recognizing a what. r? `@compiler-errors`

rustbot assigned workingjubilee Jul 11, 2023

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 11, 2023

compiler-errors reviewed Jul 11, 2023

View reviewed changes

src/tools/tidy/src/ui_tests.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

compiler-errors reviewed Jul 11, 2023

View reviewed changes

compiler-errors approved these changes Jul 11, 2023

View reviewed changes

workingjubilee reviewed Jul 11, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 11, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 26, 2023

add tidy check that forbids issue ui test filenames

13e2abf

asquared31415 force-pushed the tidy_no_issue_tests branch from fadc565 to 13e2abf Compare July 26, 2023 04:03

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 Jul 26, 2023

workingjubilee mentioned this pull request Jul 26, 2023

Add tidy rule against issue-[0-9]+.rs tests rust-lang/compiler-team#658

Closed

3 tasks

workingjubilee changed the title ~~add tidy check that forbids issue-XXXX and ice-XXXX test filenames~~ add tidy check that forbids issue-XXXX test filenames Jul 26, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 26, 2023

bors merged commit cf34adb into rust-lang:master Jul 26, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Jul 26, 2023

asquared31415 deleted the tidy_no_issue_tests branch July 26, 2023 21:29

RalfJung mentioned this pull request Jul 28, 2023

Revert "add tidy check that forbids issue ui test filenames" #114133

Merged

workingjubilee mentioned this pull request Feb 3, 2024

Reimpl meaningful test name lint MCP658 #120628

Merged

	"test file {} should not use {} in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",
	"test file `{}` should not use `{}` in its name. link the issue in a comment in the test and use a more descriptive name for the test instead.",

add tidy check that forbids issue-XXXX test filenames #113583

add tidy check that forbids issue-XXXX test filenames #113583

Conversation

asquared31415 commented Jul 11, 2023 • edited Loading

rustbot commented Jul 11, 2023

This comment has been minimized.

compiler-errors Jul 11, 2023

Choose a reason for hiding this comment

workingjubilee Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

compiler-errors left a comment

Choose a reason for hiding this comment

compiler-errors Jul 11, 2023

Choose a reason for hiding this comment

matthiaskrgr commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

Noratrieb commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023

Noratrieb commented Jul 11, 2023

workingjubilee Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

tgross35 commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023 • edited Loading

workingjubilee commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

matthiaskrgr commented Jul 11, 2023

workingjubilee commented Jul 11, 2023

asquared31415 commented Jul 11, 2023

bors commented Jul 26, 2023

workingjubilee commented Jul 26, 2023

asquared31415 commented Jul 26, 2023 • edited Loading

workingjubilee commented Jul 26, 2023

workingjubilee commented Jul 26, 2023

workingjubilee commented Jul 26, 2023

bors commented Jul 26, 2023

bors commented Jul 26, 2023

matthiaskrgr commented Jul 26, 2023

compiler-errors commented Jul 26, 2023

bors commented Jul 26, 2023

rust-timer commented Jul 26, 2023

Overall result: ❌ regressions - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

asquared31415 commented Jul 11, 2023 •

edited

Loading

workingjubilee Jul 11, 2023 •

edited

Loading

workingjubilee Jul 11, 2023 •

edited

Loading

matthiaskrgr commented Jul 11, 2023 •

edited

Loading

asquared31415 commented Jul 26, 2023 •

edited

Loading