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

no-fail-fast support for tool testsuites #108264

Merged
merged 1 commit into from
Feb 22, 2023
Merged

no-fail-fast support for tool testsuites #108264

merged 1 commit into from
Feb 22, 2023

Conversation

jchecahi
Copy link

@jchecahi jchecahi commented Feb 20, 2023 •
edited
Loading

This commit adds a change to pass "--no-fail-fast" flag to cargo test inside tool::prepare_tool_cargo() so there is no need to do it manually in each Step trait implementation in src/bootstrap/test.rs.

Also, removes the flag from test.rs where prepare_tool_cargo() is called so cargo doesn't complain because the flag has been passed twice.

This commit adds --no-fail-fast flag to each cargo test
command in each tool Step trait implementation (miri, rustfmt and clippy).

Fixes #108261

@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ozkanonur (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added 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 Feb 20, 2023
@jchecahi
Copy link
Author

I have tested this by enforcing some tests to fail by adding an assert!(1==2); statement in clippy, rustfmt and miri tests. Without the commit, the testsuite stops running after a failure, with the commit, it keeps going until the end.

@onur-ozkan
Copy link
Member

Changing behaviour of test suite in tool module doesn't seem right. It also duplicates the control testing to multiple modules which isn't really good. You can pass --no-fail-fast for Miri, Rustfmt and Clippy same as in the other Step implementations in test module.

@onur-ozkan
Copy link
Member

@rustbot author

@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 Feb 21, 2023
@jchecahi jchecahi force-pushed the tool-testsuite-ignores-no-fail-fast branch from 5db2939 to 83ae6ef Compare February 22, 2023 07:29
Add no-fail-fast support to miri, rustfmt and clippy testsuites
2f16355 
This commit adds `--no-fail-fast` flag to each `cargo test`
command in each tool Step trait implementation.

Fixes rust-lang#108261
@jchecahi jchecahi force-pushed the tool-testsuite-ignores-no-fail-fast branch from 83ae6ef to 2f16355 Compare February 22, 2023 07:30
@jchecahi
Copy link
Author

@ozkanonur thanks for the input, I've made the changes you recommended. Let me make sure I understood the reasoning behind your advice. IIUIC the changes related to the testsuites should be placed always in src/bootstrap/test.rs. Following that reasoning, if the goal is to gather this kind of duplicated logic (as suggested in #104198), the proper way to do it would be to create a function in src/bootstrap/test.rs that takes care of that. Did I understand it correctly?

@onur-ozkan
Copy link
Member

We already pass the test flags in test module, if we do it in other modules as well, it would be not good for us to work/debug on it.

And yes, creating fn could be quite nice.

@onur-ozkan
Copy link
Member

onur-ozkan commented Feb 22, 2023 •
edited
Loading

This looks good for now, we can do the code unification in seperated PR pointing the related issue. Thank you for the contribution.

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 22, 2023

📌 Commit 2f16355 has been approved by ozkanonur

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2023
@bors
Auto merge of rust-lang#108339 - GuillaumeGomez:rollup-4z02kas, r=Gui…
3b4d6e0 
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#108110 (Move some `InferCtxt` methods to `EvalCtxt` in new solver)
 - rust-lang#108168 (Fix ICE on type alias in recursion)
 - rust-lang#108230 (Convert a hard-warning about named static lifetimes into lint "unused_lifetimes")
 - rust-lang#108239 (Fix overlapping spans in removing extra arguments)
 - rust-lang#108246 (Add an InstCombine for redundant casts)
 - rust-lang#108264 (no-fail-fast support for tool testsuites)
 - rust-lang#108310 (rustdoc: Fix duplicated attributes for first reexport)
 - rust-lang#108318 (Remove unused FileDesc::get_cloexec)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8fd47a6 into rust-lang:master Feb 22, 2023
@rustbot rustbot added this to the 1.69.0 milestone Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@onur-ozkan onur-ozkan

Labels
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.69.0
Development

Successfully merging this pull request may close these issues.

x.py test ignores --no-fail-fast in some tool testsuites.
4 participants
@jchecahi @rustbot @onur-ozkan @bors