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

Tidy rule against issue-* filenames in tests? #113345

Closed
workingjubilee opened this issue Jul 4, 2023 · 9 comments
Closed

Tidy rule against issue-* filenames in tests? #113345

workingjubilee opened this issue Jul 4, 2023 · 9 comments
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-testsuite Area: The testsuite used to check the correctness of rustc

Comments

@workingjubilee
Copy link
Contributor

workingjubilee commented Jul 4, 2023

These filenames are really quite confounding as they mean that everything requires looking at its contents to understand what it is about. This kind of filename is only slightly better than naming it test.rs. We may want to lint against adding new ones.

@workingjubilee workingjubilee added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jul 4, 2023
@Mark-Simulacrum
Copy link
Member

For random ICE tests and such it's not always clear we have a better name - I think before we lint we should have guidelines on good alternatives. (Or decide that if you can't name it you shouldn't add it, but that seems strictly worse).

@workingjubilee
Copy link
Contributor Author

Yes, we have to at least have a minimum bar of "what else, then?" before we discourage them programmatically. Though perhaps we could also say that, for instance, tests in some specific folder, like rust/tests/ui/ice/**/*, are excepted from such a lint.

@jieyouxu
Copy link
Contributor

jieyouxu commented Jul 5, 2023

I think sometimes having the issue number is useful for context, but

  • issue-xxx.rs where it's only the issue number and no other info is awful for searching/discoverability and almost certainly deserve a tidy error.
  • issue-xxx-what-it-tests.rs seem alright?

@workingjubilee
Copy link
Contributor Author

workingjubilee commented Jul 5, 2023

Test files named issue-123456-ice.rs would mean the last 3 letters of the (pre-extension) filename give us more data about the test's actual purpose than the other 13 (which would serve as indirect pointer only), but perhaps that would be sufficient.

@Nilstrieb
Copy link
Member

I think every ICE has a better potential name than issue-XXX. At least for the ones I've fixed. I think including the issue number is useful, but I prefer having it as a comment in the file instead of the filename.

@jyn514 jyn514 added the A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself label Jul 5, 2023
@asquared31415
Copy link
Contributor

@rustbot claim

@asquared31415
Copy link
Contributor

Initial investigation:
3063/15424 (~19.86%) *.rs ui test files match ^issue[-_ ]?\d+$
another 1349 files (total ~28.60%) contain that pattern in addition to some other text, where they should probably omit it in favor of a comment.

Wow that's a lot of tests. I'm not sure it's feasible for this to retroactively apply to existing tests because each of these tests requires manual investigation to determine what it's meant to be testing. I suspect that a large number of the tests are very old, and the older tests, especially pre-1.0, often need much deeper investigation. This at least cannot be in one large PR, and we'd certainly want to stop new tests from being like this as soon as possible.

@workingjubilee
Copy link
Contributor Author

I believe that we should grandfather clause all the existing ones rather than simply forcing them all to migrate immediately.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 26, 2023
…=workingjubilee

add tidy check that forbids issue-XXXX and ice-XXXX test filenames

Helps with rust-lang#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`
@asquared31415
Copy link
Contributor

@rustbot release-assignment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself A-testsuite Area: The testsuite used to check the correctness of rustc
Projects
None yet
Development

No branches or pull requests

6 participants