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.

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

Remove //@ compare-output-lines-by-subset #133725

Merged
merged 1 commit into from
Dec 2, 2024
Merged

Remove //@ compare-output-lines-by-subset #133725

merged 1 commit into from
Dec 2, 2024

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 2, 2024 •
edited by jieyouxu
Loading

There was only ever one test which used this flag, and it was removed in #132244. I think this is a bad flag that should never have been added; comparing by subset makes the test failures extremely hard to debug. Any test that needs complicated output filtering like this should just use run-make instead.

Note that this does not remove the underlying comparison code, because it's still used if runner is set. I don't quite understand what's going on there, but since we still test on other platforms and in CI that the full output is accurate, I think it will be easier to debug than a test that uses compare-by-subset unconditionally.

rustc-dev-guide update PR: rust-lang/rustc-dev-guide#2151

@jyn514
Remove //@ compare-output-lines-by-subset
2f17ea0 
There was only ever one test which used this flag, and it was removed in rust-lang#132244. I think this is a bad flag that should never have been added; comparing by subset makes the test failures extremely hard to debug. Any test that needs complicated output filtering like this should just use run-make instead.

Note that this does not remove the underlying comparison code, because it's still used if `runner` is set. I don't quite understand what's going on there, but since we still test on other platforms and in CI that the full output is accurate, I think it will be easier to debug than a test that uses compare-by-subset unconditionally.
@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2024

r? @wesleywiser

rustbot has assigned @wesleywiser.
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-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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) labels Dec 2, 2024
@jyn514
Copy link
Member Author

jyn514 commented Dec 2, 2024

r? bootstrap

@rustbot rustbot assigned albertlarsan68 and unassigned wesleywiser Dec 2, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Dec 2, 2024

r? jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned albertlarsan68 Dec 2, 2024
jieyouxu
jieyouxu approved these changes Dec 2, 2024
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!

@jieyouxu
Copy link
Member

jieyouxu commented Dec 2, 2024

You can r=me after PR CI is green.
@bors delegate+ rollup

@bors
Copy link
Contributor

bors commented Dec 2, 2024

✌️ @jyn514, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@jieyouxu jieyouxu mentioned this pull request Dec 2, 2024
Merged
@jieyouxu
Copy link
Member

jieyouxu commented Dec 2, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 2, 2024

📌 Commit 2f17ea0 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2024
@Veykril
Copy link
Member

Veykril commented Dec 2, 2024 •
edited
Loading

This was added for ferrocene in #110444, back then it wasn't used by the project at implementation time either. More specifically this was used mainly for testing CLI flag output which frequently changes, I don't think modeling those a run-make tests makes sense does it? Relevant ferrocene tests https://github.com/search?q=repo%3Aferrocene%2Fferrocene+compare-output-lines-by-subset+path%3A%2F%5Etests%5C%2F%2F&type=code

Though given the actual implementation logic stays (for now) I guess we can revert this PR in ferrocene given the diff itself is small. Though the story changes of course if the implementation also goes away

@jieyouxu
Copy link
Member

jieyouxu commented Dec 2, 2024 •
edited
Loading

This was added for ferrocene in #110444, back then it wasn't used by the project at implementation time either. More specifically this was used mainly for testing CLI flag output which frequently changes, I don't think modeling those a run-make tests makes sense does it? Relevant ferrocene tests github.com/search?q=repo%3Aferrocene%2Fferrocene+compare-output-lines-by-subset+path%3A%2F%5Etests%5C%2F%2F&type=code

Though given the actual implementation logic stays (for now) I guess we can revert this PR in ferrocene given the diff itself is small. Though the story changes of course if the implementation also goes away

I don't foresee us removing the implementation because it's still needed to handle test runner output. However, I don't want to accept tests in tree with //@ compare-output-lines-by-subset because it really makes test failures hard to diagnose. (Now none of in-tree tests use this directive)

@jyn514
Copy link
Member Author

jyn514 commented Dec 2, 2024

More specifically this was used mainly for testing CLI flag output which frequently changes, I don't think modeling those a run-make tests makes sense does it?

run_make_support intentionally has support for diffing output:

https://github.com/rust-lang/rust/blob/master/src/tools/run-make-support/src/diff/mod.rs

@GuillaumeGomez GuillaumeGomez mentioned this pull request Dec 2, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2024
@bors
Auto merge of rust-lang#133760 - GuillaumeGomez:rollup-2c1y8c3, r=Gui…
d49be02 
…llaumeGomez

Rollup of 13 pull requests

Successful merges:

 - rust-lang#133603 (Eliminate magic numbers from expression precedence)
 - rust-lang#133715 (rustdoc-json: Include safety of `static`s)
 - rust-lang#133721 (rustdoc-json: Add test for `impl Trait for dyn Trait`)
 - rust-lang#133725 (Remove `//@ compare-output-lines-by-subset`)
 - rust-lang#133730 (Add pretty-printer parenthesis insertion test)
 - rust-lang#133736 (Add `needs-target-has-atomic` directive)
 - rust-lang#133739 (Re-add myself to rotation)
 - rust-lang#133743 (Fix docs for `<[T]>::as_array`.)
 - rust-lang#133744 (Fix typo README.md)
 - rust-lang#133745 (Remove static HashSet for default IDs list)
 - rust-lang#133749 (mir validator: don't store mir phase)
 - rust-lang#133751 (remove `Ty::is_copy_modulo_regions`)
 - rust-lang#133757 (`impl Default for EarlyDiagCtxt`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 15fda1e into rust-lang:master Dec 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 2, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2024
@rust-timer
Unrolled build for rust-lang#133725
0f4976f 
Rollup merge of rust-lang#133725 - jyn514:remove-compare-output-subset, r=jieyouxu

Remove `//@ compare-output-lines-by-subset`

There was only ever one test which used this flag, and it was removed in rust-lang#132244. I think this is a bad flag that should never have been added; comparing by subset makes the test failures extremely hard to debug. Any test that needs complicated output filtering like this should just use run-make instead.

Note that this does not remove the underlying comparison code, because it's still used if `runner` is set. I don't quite understand what's going on there, but since we still test on other platforms and in CI that the full output is accurate, I think it will be easier to debug than a test that uses compare-by-subset unconditionally.

rustc-dev-guide update PR: rust-lang/rustc-dev-guide#2151
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc 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)
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
7 participants
@jyn514 @rustbot @jieyouxu @bors @Veykril @wesleywiser @albertlarsan68