Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

tests/ui: A New Order [14/N] #142440

wants to merge 1 commit into from
+203 −137

Conversation

Kivooeo
Copy link
Contributor

@Kivooeo Kivooeo commented Jun 12, 2025 •
edited
Loading

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 under tests/ui/. Part of #133895.

r? @jieyouxu

@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2025

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 12, 2025
@rustbot rustbot assigned jieyouxu and unassigned lcnr Jun 12, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@speedy-lex
Copy link
Contributor

@Kivooeo, Why is this refactor split up into 14 pr’s? Wouldn’t it be better for it to be in 1 pr?

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jun 13, 2025 •
edited
Loading

@speedy-lex

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

Kivooeo
Kivooeo commented Jun 13, 2025
View reviewed changes
tests/ui/issue-15924.rs Outdated
Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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 :)

Copy link
Contributor Author

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 ;)

Copy link
Member

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?

Copy link
Contributor Author

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

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2025

There are changes to the tidy tool.

cc @jieyouxu

@rustbot

This comment has been minimized.

Sign in to view
jieyouxu
jieyouxu requested changes Jun 28, 2025
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a 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
Copy link
Member

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?

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 28, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2025

⚠️ Warning ⚠️

@Kivooeo
Copy link
Contributor Author

Kivooeo commented Jun 28, 2025

@rustbot ready

here is deleted test

https://github.com/rust-lang/rust/pull/142440/files#diff-80795699fb190e9f72b5c8ab000a3d7f2c7d464e04ba4265ca41dc82b0b0790f

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 28, 2025
@tgross35
Copy link
Contributor

tgross35 commented Jun 28, 2025 •
edited
Loading

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)

@jieyouxu
Copy link
Member

I'm going to focus on some bootstrap/compiletest changes, so
r? @tgross35 (since you're looking at this :D)

@rustbot rustbot assigned tgross35 and unassigned jieyouxu Jun 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35

@jieyouxu jieyouxu

Assignees

@tgross35 tgross35

Labels
A-tidy Area: The tidy tool S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Kivooeo @rustbot @rust-log-analyzer @speedy-lex @tgross35 @jieyouxu @lcnr