Move ui/issues tests to some subdirectories by issue number

[JohnTitor]

Helps #73494
This moves all the tests under ui/tests to reduce the number of entries per one directory, it should also reduce our frustration on GitHub UI and editor/IDE.
The actual diff (i.e. --no-renames) is quite small and can be reviewed easily.

[JohnTitor]

r? @lcnr as the idea is theirs and @petrochenkov as you work on the test structure usually.

[lcnr]

I feel like this is a step in the right direction.

As said in #73494 it would be better to somehow sort these issues by what they are testing, this is a lot of effort and I don't expect us to make much progress there in the near future, if ever. So it's better to make some incremental improvements here

While it is a fairly big change when looking at it's size, it is fairly easy to revert and doesn't really lock us into anything, so I do not think an MCP is needed here. cc @rust-lang/compiler-contributors

As @JohnTitor also requested a review by @petrochenkov I will wait a bit until actually merging this change by myself.

[petrochenkov]

This is a step in a very wrong direction, and organizing tests by their issue number doesn't make any sense.
Grouping should be semantic and feature-based, and creating artificial grouping like this only creates churn and less incentive to organize the test suite properly.

[matklad]

strongly agree with @petrochenkov. Naming the test just by issue number is an anti-pattrn in itself, we shouldn't execebrate it. If anything, i suggest opening an e-mentor issue to rename issuexxxx.rs tests to format-bad-args-issuexxxx.rs and implementing a tidy check to enforce semantic names.

[lcnr]

This is a stop in a very wrong direction, and organizing tests by their issue number doesn't make any sense.

I quite strongly disagree here. Doing this improves directory traversal both on github and in an IDE, so in my view this is a definite improvement over the current state. Github does not fully show directories with more than 1000 files which is currently the case for the ui/issues folder.

Grouping should be semantic and feature-based

Ideally yes, but we both know that this is currently not the case for the ui/issues folder.

creating artificial grouping like this only creates churn

The churn is really small though, or am I missing something here? There are some PRs rn which end up changing the error messages for some of the tests here so these would have to rebase but I do not think that this is a big issue, especially because in my experience git is clever enough to deal with simple file movements. I expect these to be just git rebase origin/master which does seem worth the improved directory navigation in the long run.

and less incentive to organize the test suite properly.

Do you expect us to organize the test suite properly if we do not merge this? Because for me there isn't the choice between this change and organizing 1600 tests properly, but instead between doing nothing and actually being able to use the github ui to navigate these directories.

If you don't think this change improves on the current situations i also don't see how this would remove an incentive to change this in the future.

edit: @matklad does this PR prevent us opening a issue like this?

[matklad]

edit: @matklad does this PR prevent us opening a issue like this?

I don't think this PR precludes filing such an issue, and I don't think it'll increase the momentary workload to complete it.

However, I feel that this PR will greatly increase incentive to name future test cases just issuexxxx.rs, I think it creates conformance pressure in the wrong direction.

I wonder if this needs a more MCP-like process? It's clear that there are at least two problems with ui testsuite:

• semantic org is poor
• some directories (notably, the top-level one) are overly large

I think we can benefit from brainstrorming the problem space instead of just jumping directly to a solution.

[petrochenkov]

Because for me there isn't the choice between this change and organizing 1600 tests properly, but instead between doing nothing and actually being able to use the github ui to navigate these directories.

The third alternative is to start moving the tests into directories right now instead of arguing.
I'm ready to spend half an hour today tomorrow to check how many tests I can move in that time, I'm sure there are many tests for which relevant directories already exist and we only need to move the file (and give it a semantic name preferably, but that's extra work).

[lcnr]

I think we can benefit from brainstorming the problem space instead of just jumping directly to a solution.

yeah, but this PR isn't a solution, it's a hopefully temporary band-aid. I don't want to ignore easy improvements for the sake of keeping people motivated to work on a more optimal one.

I'm ready to spend half an hour today to check how many tests I can move in that time

If you have the time to do this go ahead, I might be overestimating the required work for this so some additional data could be insightful 👍

[matklad]

I don't want to ignore easy improvements

I think there's a disagreement about whether this is an improvement or not. I worry that, by making adding issue tests easier, we'll create an incentive to add more of these tests.

I am also skeptical of the size of the improvement here -- ui/issues/ is the leaf directory, there isn't much benefit in making it more structured.

This is in contrast to the u/ non-leaf directory, where a huge number of test files does create problems. Moving ui/*.rs to ui/yet-unclassified/ would be an easy win imo

[bors]

☔ The latest upstream changes (presumably #79697) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[petrochenkov]

@davidtwco W H Y ? !

[petrochenkov]

I wonder if this needs a more MCP-like process? It's clear that there are at least two problems with ui testsuite:

• semantic org is poor

After looking at the existing subdirectories, this is indeed a problem.
Looks like one of the initial approaches behind creating the subdirectories was "let's organize tests into a trie", which makes approximately as much sense as issue-10000-19999.

[lcnr]

@davidtwco W H Y ? !

I do not think that comments like this are in any way helpful.

After looking at the existing subdirectories, this is indeed a problem.
Looks like one of the initial approaches behind creating the subdirectories was "let's organize tests into a trie", which makes approximately as much sense as issue-10000-19999.

Yeah, and as i think that issue-10000-19999 is better than no hierarchy at all. Sorting tests by what they are testing is a hard problem and I currently believe that a strong push to sort these more sensibly is not really worth the effort.

Especially people unfamiliar with the compiler often can't tell what a test is supposed to test and people who do understand the relevant area well enough often have to deal with issues which are more relevant at that moment.

[davidtwco]

@davidtwco W H Y ? !

I can't remember, the PR was two years ago and moved 1500+ tests between suites. Grouping by the prefix bad is admittedly, well, bad, but it's nothing which can't be corrected. I'm also interested in seeing our tests better organized and having a better structure for that, but I'm not sure that leaving this comment furthers that goal.

While I'm here, I agree with @lcnr that landing this PR is worthwhile - it doesn't solve the problem of unorganized tests, but it makes using GitHub's file viewer feasible for our tests - that's useful. Disorganized tests are going to be resolved by someone going away and organizing the tests and then implementing a tidy lint to prevent new tests named like this, not by preventing incremental improvements to the status quo (which I don't believe, in this case, incentivizes or disincentivizes an actual solution to this).

I am more than happy to help you in fixing our disorganized tests, let's chat about it on Zulip and work out a way to divide the work up and actually solve that problem.

[tmandry]

A common pattern I've seen for transitions like this is to implement something like the tidy lint first, using an allowlist to accommodate all existing cases. This prevents the problem from growing further without requiring someone to fix it all at once. (Which is often impossible for one person to do, due to lack of context on every part of the codebase.)

[petrochenkov]

I'm ready to spend half an hour today tomorrow to check how many tests I can move in that time

#79776 is the result.
30 minutes on actually moving the tests + some time on running the test suite, moving auxiliary files and otherwise making sure that all the tests pass.
139 files touched, mostly from src/test/ui/issues but some from src/test/ui root as well.

[lcnr]

I recently once again manually searched for a file in ui/issues which is fairly annoying rn. I can spend a few minutes to figure out how to search this more efficiently. The same is however also true for other, especially new, contributors and is another unnecessary hurdle people have to overcome.

@petrochenkov thanks for opening #79776. It seems that it took about 45 minutes of contributor time to write and merge this PR. Considering that these were the "easier" ones to move, I guess doing so for the remaining tests would require an additional 10 to 20 hours. I currently don't have the capacity to either do this myself or open a E-mentor tracking issue so I am still in favor of landing this PR as is.

In case you still don't believe that doing so is acceptable I would be in favor of having a t-compiler meeting about this at the start of next year and simply closing this PR.

[petrochenkov]

Some update from my side is that last weekend I wanted to write the classifier (#73494 (comment)) to see how well it performs, but spent all the time on reviewing and didn't get to it.

I recently once again manually searched for a file in ui/issues which is fairly annoying rn. I can spend a few minutes to figure out how to search this more efficiently.

Could you describe the use case in some more details? What did you try to find?

I don't think I ever had a need to go through the whole file list in "ui" root or "ui/issues" in GitHub interface (that's the reason why I don't consider the issue urgent), and for text search or file name search the number of files doesn't matter.

[petrochenkov]

One more idea I had is making a table mapping error codes (E0123) to subdirectories in some form.
This way beginner contributors will now where to place their test, at least if it's a "fail" test.
(This if kind of a reduction of the idea with classifier to a single feature.)

[lcnr]

Could you describe the use case in some more details? What did you try to find?

I tried to find issues-54335.rs and manually searched the directory for that.

I can spend a few minutes to figure out how to search this more efficiently

Every IDE should have a feature to search a file without doing so, but that's something people have to learn first. Once you use that feature it is true that there pretty much no reason to wade through ui/issues to find a test. We must however not assume that everyone does so. I mean look at me ✨

One more idea I had is making a table mapping error codes (E0123) to subdirectories in some form.
This way beginner contributors will now where to place their test, at least if it's a "fail" test.
(This if kind of a reduction of the idea with classifier to a single feature.)

This does seem like a good way to quickly solve this a bit more cleanly while also being fairly straightforward, so I wouldn't mind someone opening an issue for that instead.

[JohnTitor]

My initial thought was the current number isn't IDE friendly and this would be a stop-gap, as @lcnr said, but I understand some folks don't think it makes sense.
Since we implemented the tidy check (#79959) and there are some moving PRs (#79776, #80539), I think the issue will be fixed eventually. I'm going to close this as outdated and rejected, if we want some more discussion, I'd like to see a Zulip stream for it :)