-
Notifications
You must be signed in to change notification settings - Fork 13.5k
tests/ui
: A New Order [14/N]
#142440
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
base: master
Are you sure you want to change the base?
tests/ui
: A New Order [14/N]
#142440
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@Kivooeo, Why is this refactor split up into 14 pr’s? Wouldn’t it be better for it to be in 1 pr? |
This is part of a long-term cleanup initiative tracked in #133895 The PRs are intentionally small — around 5–7 tests each — to make them easier to review and avoid overwhelming maintainers |
tests/ui/issue-15924.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: i mistakenly deleted issue-15924.rs (#15924) thinking it didn't reproduce the original problem. Should I restore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only reason i'm asking is because of this comment #15924 (comment), seems like there is no way to reproduce this anymore because of "new interface scheme", making the regression test potentially obsolete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, regression tests should err on the side of sticking around. Not being able to reproduce is a good thing - but just because the code is refactored doesn't mean a new design can't hit the same problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so preferably to keep this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a solid choice when in doubt :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure! thanks for the answer ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something, or is this supposed to be kept?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No :) You don't I was too lazy to add it in-place and then forget, I will return it now with squash
There are changes to the cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, some minor nits, and a licensing problem for a test
tests/ui/issue-15924.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something, or is this supposed to be kept?
This comment has been minimized.
This comment has been minimized.
Please split the renames to a separate commit as discussed in #143118; six tests are losing their history here :( (happy to do it for you if you get in a rebase rut) |
I'm going to focus on some bootstrap/compiletest changes, so |
☔ The latest upstream changes (presumably #143233) made this pull request unmergeable. Please resolve the merge conflicts. |
@tgross35 may I ask you to make the same thing as you do for 15th part |
This comment has been minimized.
This comment has been minimized.
How I did this, for reference # Check out your branch
git switch tf14
# Save this commit for later
old_tf14="$(git rev-parse tf14)"
# Make your branch exactly like `master` it branched from
git reset --hard $(git merge-base tf14 master)
# Do all the renames (this was the trickiest part, figuring out how they match up)
git mv tests/ui/kinds-in-metadata.rs tests/ui/cross-crate/metadata-trait-serialization.rs
git mv tests/ui/issue-15924.rs tests/ui/higher-ranked/higher-ranked-encoding.rs
git mv tests/ui/issues-71798.rs tests/ui/async-await/impl-future-escaping-bound-vars-ice.rs
git mv tests/ui/issues-71798.stderr tests/ui/async-await/impl-future-escaping-bound-vars-ice.stderr
git mv tests/ui/auxiliary/issue-16822.rs tests/ui/cross-crate/auxiliary/cross-crate-refcell-match.rs
git mv tests/ui/auxiliary/kinds_in_metadata.rs tests/ui/cross-crate/auxiliary/kinds_in_metadata.rs
git mv tests/ui/issue-16822.rs tests/ui/cross-crate/cross-crate-refcell-match.rs
git mv tests/ui/item-name-overload.rs tests/ui/modules/mod-same-item-names.rs
...
# I took the tidy update here too, could really go in either
git checkout "$old_tf14" -- src/tools/tidy/src/issues.txt
# Commit this with a new message
git commit -a
# Now make everything look exactly like your branch
git checkout "$old_tf14" -- .
# And create a new commit using your existing commit message
git commit -a -C "$old_tf14"
# Sanity check that things look correct (empty diff, first commit only contains moved files
# and the tidy fix)
git log -2 --stat
git diff "$old_tf14"
# And update the PR
git push ... --force-with-lease It looks like there is a conflict in the tidy list, I'll let you resolve that. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Prepare for rework done by the rest of RUST-142440. Co-authored-by: Kivooeo <Kivooeo123@gmail.com>
Note
Intermediate commits are intended to help review, but will be squashed prior to merge.
Some
tests/ui/
housekeeping, to trim down number of tests directly undertests/ui/
. Part of #133895.r? @jieyouxu