Closed workingjubilee closed 1 year ago
rustbot
commented
1 year ago This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
cc @rust-lang/compiler @rust-lang/compiler-contributors
compiler-errors
commented
1 year ago Strongly agree that we shouldn't be committing issue-###.rs pretty much ever.
PR authors who are committing tests with their PR should leave at least a couple of words either relating the test to the PR's changes (e.g. check-static-values-constraints.rs), or explaining what the test actually exercises (e.g. illegal-upcast-from-impl.rs).
The only case where we don't "really know" what to name a test other than issue-###.rs is when a issue goes from ICE/fail -> pass and it's only caught by manual inspection or glacier. But with the existence of cargo-bisect-rustc, we should be able to find out what PR fixes the issue, and then work backwards from there to give the test a better name.
@rustbot second
RalfJung
commented
1 year ago To clarify, the information about the issue still absolutely must be present, right? Having a comment in the file "regression test for issue XXX" is not optional, if the file name no longer indicates the number. Often the issue contains a lot more valuable information than can be put into the filename.
oli-obk
commented
1 year ago Yes, ideally all tests have a doc comment at the top explaining its context (with links/ids) and its purpose in a bit more than a file name can convey.
matthiaskrgr
commented
1 year ago If we could, I would like to make it mandatory to have an issue number in the file name but we probably can't enforce that without it being a PITA for false positives.
If you add a new language feature in pr 12345 and all your files that are already in a ${feature_name} directory having to be named trait-impl-timelifes-12345.rs that is probably not helpful.
If we forbid issue-[0-9]+.rs should we also have an eye out for ice-1234.rs, pr456.rs or 6789.rs ?
(I think it is kind of ironic that 1235.rs which is obviously worse would NOT be flagged by the regex as it is now :upside_down_face: )
The previous pr said "UI test{}should use a name that describes the test and link the issue in a comment instead.", in the warning, which sounds to me like we would disencourage people from adding the issue number to the file which I am very much against.
One of my usecases is that when I check for new compiler crashes^1, and I see that test21345.rs causes an ICE; I can immediately just look up 21345 in the issue tracker to get an idea if the original issue was already fixed or is still open, if there is a PR for it, without having to manually open files, which does not sound like a lot but a regular run just across a normal rustc repo can already yield around 200-300 findings of all sorts, so it definitely adds up.
I would prefer we encourage a naming scheme where we include an issue number next to a short human readable description like 10983-crash-during-normalizsation-of-inf-recursion.rs or malformed-lifetime-in-whatever-1234.rs
Having a machine readable way of storing issue ids inside the files also sounds nice but should not rule out issue numbers in filenames imo.
bryangarza
commented
1 year ago I would prefer we encourage a naming scheme where we include an issue number next to a short human readable description like
10983-crash-during-normalizsation-of-inf-recursion.rsormalformed-lifetime-in-whatever-1234.rs
Yeah rather than excluding certain filename formats (which may miss variations as you said), it'd be nice to instead enforce a consistent format like what you described. Then going forwards, all the tests will be both human-readable and machine-readable.
workingjubilee
commented
1 year ago This is discussion in the issue and not in the Zulip stream, rather than summarizing that discussion or anything else, and I have avoided responding in a further reply to this issue because it is against protocol to do so.
apiraino
commented
1 year ago @rustbot label -final-comment-period +major-change-accepted
Proposal
WHEN in the Course of human events, it becomes necessary for some programmers to dissolve the bad decisions which have connected them with their bad habits, a decent respect to the opinions of humanity requires that they should declare the causes which impel them to the separation.
We hold these truths to be self-evident:
The tests in
tests/ui/have long had a habit of overwhelming contributors with increasingly meaningless series of integers, in the pattern ofissue-[0-9]+.rs, so it is impossible to identify which tests are for what. They have further injured contributors by sorting unrelated issues adjacent to each other, making it impossible to further organize them except by manual inspection. And they have made it so it is impossible to identify relationships between tests if one does not cross-reference the issues themselves, which is not easy without reference to a source external to the git repository per se.THEREFORE
we herewith propose that such names be abolished permanently from the codebase going forward, if they cannot add so much as a single additional iota of information that will be useful to the human eye to disambiguate them. Further, that these issue numbers, if they are placed anywhere in the file name, should be sorted towards the end and not the beginning of the file name.
Mentors or Reviewers
Myself. See also:
Process
The main points of the Major Change Process are as follows:
@rustbot second.-C flag, then full team check-off is required.@rfcbot fcp mergeon either the MCP or the PR.You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.