Clean up some tests in tests/ui

[spencer3035]

I cleaned up 3 top level tests, keeping the changes minor because it is my first PR and wanted to get feedback before doing more changes/PRs.

Tracking issues:
#73494
#133895

r? jieyouxu

[jieyouxu]

Thanks for the PR! Always glad to see test cleanup/improvement efforts.

I left a lot of feedback on individual test changes since you were looking for feedback (to maybe guide your future test changes), and some of them are way more nitpicky than I may fixate on normally.

I reviewed your changes commit-by-commit, so my review comments are best viewed commit-by-commit too.

I'll also point out a commit history nit here: to make it easier during review, I would structure the commits like:

• Commit 1 renames test X (maybe move directory and/or rename).
• Commit 2 fixes/documents test X.

Notably, I would rebase and fixup test stdout/stderr reblesses to the second "fixes/documents" commit. Then when we decide together after iterations that the test changes look good, I'll ask you to adjust the commit history to have 1 commit per individual test (this is to help future git archaeology).

BTW, you can also add in your PR descriptions that your PR is "Part of #133895" efforts 😁

[spencer3035]

I tried to keep the history linear for now. In the end, we want to rebase to one commit per test correct?

Edit: I read your comment above that answers this question.

[jieyouxu]

Cool. BTW you can mark your PR as ready for review via @rustbot ready (see https://rustc-dev-guide.rust-lang.org/contributing.html#waiting-for-reviews), otherwise I use the S-waiting-on-xx to filter out PRs in my review queue that needs action from me (i.e. review).

[jieyouxu]

Thanks! The changes themselves look good, I left a few comment nits but no real functional changes.

At this point in the review, I'm fairly confident that the changes are mostly ready to merge to master, so I am requesting that you adjust the commit history to have 1 commit for each test, e.g. sth like

• Commit 1 improves outer-mod-attr-... test
• Commit 2 improves duplicate lang item test
• Commit 3 improves duplicate label test

To help future git archaeology. If these tests were actually (closely) related then I would've requested just to squish them into one commit, but that's not the case here, which is why I am requesting one commit per different test.

[jieyouxu]

(I resolved all review comments from the previous review pass because I found them (more than) adequately addressed, but you may wish to expand a few of them since I also left some extra info/feedback. GitHub likes to hide the resolved comments so just wanted to let you know.)

[spencer3035]

I addressed the comments and rebased to one commit per test. This process has made me way better at modifying git history and organizing commits.

I'm assuming the CI will pass because it passed locally.

@rustbot ready

[jieyouxu]

Looks good, thanks!

[jieyouxu]

I'm assuming the CI will pass because it passed locally.

Not necessarily the case, because CI could have some environmental differences (e.g. platforms) versus local environments. But the tests here are mostly target/env-agnostic so they should be fine.

Anyway,

@bors r+ rollup

[bors]

📌 Commit 27077b9 has been approved by jieyouxu

It is now in the queue for this repository.