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

Clean UI tests 2 of n #138639

Merged
merged 1 commit into from
Mar 22, 2025
Merged

Conversation

spencer3035
Copy link
Contributor

Modified 4 tests in tests/ui. Cleaned 3 and deleted one.

I have a final commit changing the values in src/tools/tidy/src/ui_tests.rs.
I wasn't sure if it was best practice to change this value as you go along or
once at the end. I can rebase to something that incrementally changes the value
in the "cleaned" commits if that is preferred.

Related Issues:
#73494
#133895

r? jieyouxu

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

Could not assign reviewer from: jieyouxu.
User(s) jieyouxu are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @compiler-errors

rustbot has assigned @compiler-errors.
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 Mar 18, 2025
@spencer3035
Copy link
Contributor Author

Note on modification of issues/issue-9382.rs:

This test diverged a lot from it's original intention as updates to Rust were
made. Originally, the intention was to check a compiler error for different
interactions of the ~ operator and the & operator, but ~ has since been
replaced with Box, which has it's own suite of tests. I made the decision to
keep it as a test of coercion from a &'a Vec::new() to &'a [] because I
couldn't find a similar test. I would almost remove the link to the issue in
the source because it isn't really relevant except for historical reasons.

Quoted issue: #9382
Original Commit: 920ca61

Excerpt of what the struct definitions looked like originally.

struct Thing1<'self> {
    baz: &'self [~int],
    bar: ~u64,
}

struct Thing2<'self> {
    baz: &'self [~int],
    bar: u64,
}

@jieyouxu jieyouxu assigned jieyouxu and unassigned compiler-errors Mar 18, 2025
@jieyouxu
Copy link
Member

I wasn't sure if it was best practice to change this value as you go along or
once at the end. I can rebase to something that incrementally changes the value
in the "cleaned" commits if that is preferred.

At the end is best, otherwise there's a bunch of unnecessary number changes that is super prone to merge conflicts.

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 for the PR! I left some feedback.

@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 Mar 18, 2025
@compiler-errors
Copy link
Member

Can you make sure this is squashed into one commit at the end? There's no reason that this needs 8 commits to change 8 test files.

@spencer3035
Copy link
Contributor Author

Addressed the feedback

@rustbot ready

@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 Mar 21, 2025
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.

One comment nit, then please squash the commits into one

@jieyouxu jieyouxu 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 Mar 21, 2025
@spencer3035 spencer3035 force-pushed the clean-ui-tests-2-of-n branch from 3f5a8aa to 5e6b459 Compare March 22, 2025 04:58
@spencer3035
Copy link
Contributor Author

Squashed the commits and applied minor change to wording of coercion/struct-literal-field-type-coercion-to-expected-type.rs.

@rustbot ready

@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 Mar 22, 2025
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!

@jieyouxu
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 22, 2025

📌 Commit 5e6b459 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2025
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Mar 22, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#138609 (Add stack overflow handler for cygwin)
 - rust-lang#138639 (Clean UI tests 2 of n)
 - rust-lang#138773 (catch_unwind intrinsic: document return value)
 - rust-lang#138782 (test(ui): add tuple-struct-where-clause-suggestion ui test for rust-lang#91520)
 - rust-lang#138794 (expand: Do not report `cfg_attr` traces on macros as unused attributes)
 - rust-lang#138801 (triagebot: add autolabel rules for D-* and L-*)
 - rust-lang#138804 (Allow inlining for `Atomic*::from_ptr`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f01d096 into rust-lang:master Mar 22, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 22, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2025
Rollup merge of rust-lang#138639 - spencer3035:clean-ui-tests-2-of-n, r=jieyouxu

Clean UI tests 2 of n

Modified 4 tests in tests/ui. Cleaned 3 and deleted one.

I have a final commit changing the values in `src/tools/tidy/src/ui_tests.rs`.
I wasn't sure if it was best practice to change this value as you go along or
once at the end. I can rebase to something that incrementally changes the value
in the "cleaned" commits if that is preferred.

Related Issues:
rust-lang#73494
rust-lang#133895

r? jieyouxu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tidy Area: The tidy tool S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants