tests/ui: A New Order [1/N]

[Kivooeo]

not sure if i should say something about changes here, just part of #133895

but this is my very first time doing something like this, id love to keep contributing in this area later on, so any feedback is appreciated

also should say that im going to squash it after agreement on changes

r? @jieyouxu

mind if i name this PR series like "tests/ui: A New Order [N/N]", im not sure if it fits the project tone, so id like your approval first — but i think it sounds really neat (Star Wars reference)

this could be a first part :)

[compiler-errors]

pls make sure that these commits get squashed b/c we really don't need a more than one commit for every file move, since they seem to me pretty self-descriptive :>

(edit: didn't read the "also should say that im going to squash it after agreement on changes" -- thanks for mentioning it, sorry for the ping!)

[Kivooeo]

@compiler-errors sure, i will squash them as soon as the review is done

[jieyouxu]

Some meta remarks before I look at the actual changes since you said

but this is my very first time doing something like this, id love to keep contributing in this area later on, so any feedback is appreciated

During review time, it's fine to not have so many intermediate commits, but a good granularity I find is one commit for each test (say tests/ui/foo.rs and its associated snapshots or auxiliaries or support files). This is primarily to make it easy to look at if you move and/or rename the test, along with other changes.

• Also, f31c93c is an example where it makes review more difficult. Please try to keep test reblessing within the same logic commit during review, e.g. say if you are changing tests/ui/foo.rs with a rename / doc change / reformat, also rebless foo.rs in that same commit. That way, the reviewer can look at the stderr/stdout snapshot and make sure nothing is unexpectedly changed.

You can also put a [WIP] in PR title while its not ready for merge, and/or include sth like

> [!NOTE]
>
> Intermediate commits are intended to help review, but will be squashed prior to merge.

Note

Intermediate commits are intended to help review, but will be squashed prior to merge.

What I try to do with my PR descriptions is to keep it focused about the proposed changes itself, and then leave (meta) discussions to the comments.

not sure if i should say something about changes here

You don't really need to if the changes are self-explanatory (say in commit messages or is clear from test comments). I would highlight stuff that the reviewer should pay extra attention to. Of course, backlinking to #133895 is fine.

mind if i name this PR series like "tests/ui: A New Order [N/N]", im not sure if it fits the project tone, so id like your approval first — but i think it sounds really neat (Star Wars reference)

We don't really have requirements on PR title as long as it's not completely misleading or whatever. If you'd like to use that for your series of PRs, I have no objections to that.

[jieyouxu]

Thanks, I have some suggestions

[jieyouxu]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Kivooeo]

@jieyouxu would it fine if i make this changes in one commit? or it still better to keep in separate commits

is there a strategy to find a related issue for a test?

[jieyouxu]

would it fine if i make this changes in one commit? or it still better to keep in separate commits

If I were writing this PR, I would use interactive rebase to restructure my commits to one per test (during review time, prior to merge). But if that's not something you're familiar with, I don't mind if you just squash everything into one commit, as I've seen the correspondence now.

is there a strategy to find a related issue for a test?

Yes, git blame, since it usually goes back to the original commit that introduced the test. Hopefully that indicates the corresponding issue.

[compiler-errors]

@jieyouxu: In general if this is going to be major ongoing work to add descriptions to test file, I'd pretty strongly prefer if we don't have hundreds of single file commits in the commit history just for cosmetic changes to tests. We have quite a few tests in the repo as I think we all know 😹.

[jieyouxu]

In general if this is going to be major ongoing work to add descriptions to test file, I'd pretty strongly prefer if we don't have hundreds of single file commits in the commit history just for cosmetic changes to tests. We have quite a few tests in the repo as I think we all know 😹.

Oh to clarify, I do not mean to merge a bunch of per-test commits. I only mean during review, as trying to identify the correspondence between moved and changed tests is a PITA. I was going to ask to squash before merge. This is what I meant by

During review time

[compiler-errors]

Ah, (side note maybe it's not actually worth having the convo here then) that seems like a bit of work for the author of the PR 💀 but up to the author I guess!

[Kivooeo]

@jieyouxu well, i updated everything, if it's good to you, i can squash into one commit then

[jieyouxu]

@jieyouxu well, i updated everything, if it's good to you, i can squash into one commit then

BTW you can resolve my review comments if you have addressed them 😁

[jieyouxu]

In any case, this seems fine now, please squash and we can send it off to the queue.

[Kivooeo]

@rustbot ready

[jieyouxu]

Thanks
@bors r+ rollup

[bors]

📌 Commit afc6424 has been approved by jieyouxu

It is now in the queue for this repository.