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

Properly categorize RFC UI tests #86707

Closed

Conversation

inquisitivecrystal
Copy link
Contributor

@inquisitivecrystal inquisitivecrystal commented Jun 29, 2021

For some reason, there are a bunch of folders under src/test/ui named rfc-####-*, despite the fact that there is already a src/test/ui/rfcs subdirectory for them. This is a bad thing, as src/test/ui already has an unmanageably large number of files and directories. This PR moves the RFC test subdirectories into the src/test/ui/rfcs space.

I should mention that there were a few cases with both a src/test/ui/rfc-####-* directory and a src/test/ui/rfcs/rfc-####-* directory. I resolved any collisions that resulted by renaming the files involved.

I suspect the current division came about accidentally when some other test suites were merged into UI.
r? @JohnTitor

Since you were kind enough to review my last test PR, I thought it might make sense to ask you to review this one. Do you mind being asked for reviews on PRs that sort tests? I'm planning to do some more in the future.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2021
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

rfcs is a pointless subdirectory that shouldn't exist, IMO, it's better to move the small rfcs-* subdirectories to more meaningful subdirectories corresponding to compiler areas and people maintaining them - #84065 (comment).

cc #73494 (the big project for classifying tests)

@inquisitivecrystal
Copy link
Contributor Author

inquisitivecrystal commented Jun 29, 2021

rfcs is a pointless subdirectory that shouldn't exist, IMO, it's better to move the small rfcs-* subdirectories to more meaningful subdirectories corresponding to compiler areas and people maintaining them - #84065 (comment).

cc #73494 (the big project for classifying tests)

Also, if I may ask, who's the best person to r? on test classification PRs?

Edit: there was more to this post, but I misunderstood some stuff. I'll write a revised post later.

@JohnTitor
Copy link
Member

Yeah, usually @petrochenkov handles such a PR so they should be a good reviewer here, but I'm also happy to review if it's minor (e.g. it's small or just adds a test).
And I agree with their opinion, in that case, we should move two or so (sub)directories at once so that reviewing would be easy.
(By the way, it makes me think about having README on src/test/ui to give some direction because the contributors may be confused if RFC dir doesn't exist anymore, for example.)

@bors
Copy link
Contributor

bors commented Jul 3, 2021

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

@inquisitivecrystal
Copy link
Contributor Author

Alright, it sounds like I should close this.

Before I do though, I should make sure I understand what I'd need to do to fix it. What test subdirectories are meaningful? Is there any trick, beyond the classifier tool, for figuring out which subdirectory a test should be in?

@JohnTitor
Copy link
Member

What test subdirectories are meaningful? Is there any trick, beyond the classifier tool, for figuring out which subdirectory a test should be in?

It's not documented, that's why I said we needed README (or something else). AFAIK here's a brief list of meaningless subdirs (we might have more, I haven't taken a closer look): #73494 (comment)

@inquisitivecrystal
Copy link
Contributor Author

inquisitivecrystal commented Jul 14, 2021

@JohnTitor Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants