Skip to content

Record failed tests with --record, and rerun them with --rerun#154586

Open
jdonszelmann wants to merge 4 commits intorust-lang:mainfrom
jdonszelmann:record-rerun
Open

Record failed tests with --record, and rerun them with --rerun#154586
jdonszelmann wants to merge 4 commits intorust-lang:mainfrom
jdonszelmann:record-rerun

Conversation

@jdonszelmann
Copy link
Copy Markdown
Contributor

@jdonszelmann jdonszelmann commented Mar 30, 2026

View all comments

This adds two parameters to x test:

--record

Writes a file, by default build/failed-tests, but this can be overwritten with

[build]
record_failed_tests_path = "somepath"

with a list of all tests that fail that run.

--rerun

Looks for the failed-tests file, parse it, and attempt to rerun only those tests. No cli-arguments are necessary, i.e.

x test tests/ui --record
x test --rerun

Will run all failed uitests. No need to pass tests/ui to the rerun invocation.

The last commit is a little awkward, but I think it's the best way to make it so that we first run all tests that have to be rerun, and then rerun tests passed through the cli.

This makes it so:

x test tests/ui --rerun

will first rerun failed tests, some of which may be uitests, if any fail it quits and reports failed tests, but if all pass it will run all normally passed tests. In other words, only if all previously-failed tests pass on the rerun, we then also run uitests.
Without the last commit, this would instead just run all uitests, since the failed tests form a subset of all uitests. I think that's less useful.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 30, 2026

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@rustbot rustbot added 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 Mar 30, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Mar 30, 2026

r? @clubby789

rustbot has assigned @clubby789.
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

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: bootstrap
  • bootstrap expanded to 6 candidates
  • Random selection from Mark-Simulacrum, clubby789, jieyouxu

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Copy Markdown
Member

jyn514 commented Mar 30, 2026

Writes a file, by default build/failed-tests, but this can be overwritten with

I wouldn't make this configurable.

What does x test --rerun do if there was no previous --record run? How does cache invalidation work for the failed-tests file, do you just keep it around forever?

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

@jyn514

What does x test --rerun do if there was no previous --record run?

It will warn, but treat it as if the file was empty.

How does cache invalidation work for the failed-tests file, do you just keep it around forever?

Keeps it around forever, or at least until you --record again. It prints that it's rerunning, and from what file so it should be easy to delete or even edit manually. Rerunning acts pretty much as if you passed the test paths on the cli, though we do run them before the cli-passed paths.

@rustbot

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@WaffleLapkin
Copy link
Copy Markdown
Member

Rerunning acts pretty much as if you passed the test paths on the cli, though we do run them before the cli-passed paths.

If the build/failed-tests is empty/non-present, does that make x test --rerun run all tests?

@WaffleLapkin
Copy link
Copy Markdown
Member

This is purely a speculation I suppose, but I feel like I will often forget to use --record, before the tests already failed. Would it make sense to save failures of the last command to a separate file and allow moving them to build/failed-tests?

@jyn514
Copy link
Copy Markdown
Member

jyn514 commented Mar 30, 2026

@WaffleLapkin as long as you don’t modify the compiler, compiletest will remember which tests succeeded the last time, so if you add ––failed it won’t take very long at all to regenerate that list.

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

I agree with jyn here, shouldn't make such a big difference

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

Rerunning acts pretty much as if you passed the test paths on the cli, though we do run them before the cli-passed paths.

If the build/failed-tests is empty/non-present, does that make x test --rerun run all tests?

that is true I think. What do you expect the behavior to be? maybe if no file is found, and no paths are given, exit, but if some paths are explicitly given run the explicit ones?

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

with a warning of course

@jyn514
Copy link
Copy Markdown
Member

jyn514 commented Mar 30, 2026

I think x test --rerun with no previous --record should behave exactly the same as x test.

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

mhm, well that's the current behavior. Except the warning of course, that you passed --rerun with nothing to rerun. But I think that's nice

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

cc @jieyouxu (you self assigned the other one, that one was in preparation for this one, also I figured out that bug for this one)

@clubby789
Copy link
Copy Markdown
Contributor

I haven't looked at the full implementation yet, but why not always record failed tests?

@jyn514
Copy link
Copy Markdown
Member

jyn514 commented Mar 31, 2026

Because a later invocation might only rerun a subset of tests. Say you have this series of invocations:

$ x test ui
# ... 34 failures in ui/linkage and ui/attributes
$ x build library # assume there was a compiler change
$ x test --rerun tests/ui/attributes
# ... all tests now pass
$ x test --rerun tests/ui/linkage
# ... no tests are run :( we expected all the failed linkage tests to rerun.

@clubby789
Copy link
Copy Markdown
Contributor

Sorry, always *except when using --rerun 😅

@jieyouxu jieyouxu self-assigned this Mar 31, 2026
@jdonszelmann
Copy link
Copy Markdown
Contributor Author

I still think it's a bad idea to always record: I want it to be explicit when this file gets overwritten. I might want to

x test tests/ui --record
x test tidy
x test --rerun

That's just an example, but in general what I want this feature to solve is that ignored tests get invalidated by rebuilding the compiler because of a change, something implicit. I want this recording to be explicit so you're in control of it. So you can even temporarily switch branches to try something out and come back and have your little set of tests that make your new feature break.

The last part, about switching branches is a little idea I had: we could at some point, not now, try to integrate this with git and see if we can detect branch changes and warn when you overwrite or something. Or even write to a different file (without checking it into git) etc. but if we do logic like that, that's for later

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 6, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@jdonszelmann
Copy link
Copy Markdown
Contributor Author

@jieyouxu @clubby789 rebased on the other PR now that it's merged

@rust-log-analyzer

This comment has been minimized.

Copy link
Copy Markdown
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.

(Initial review pass)

View changes since this review

Comment thread src/bootstrap/src/core/build_steps/test/failed_tests.rs
Comment thread src/bootstrap/src/core/build_steps/test/failed_tests.rs
Comment thread src/bootstrap/src/core/build_steps/test.rs
Comment thread src/bootstrap/src/core/config/flags.rs Outdated
Comment on lines +71 to +78
let trimmed = line.as_str().trim();
let without_revision =
trimmed.rsplit_once("#").map(|(before, _)| before).unwrap_or(&trimmed);
let without_suite_prefix = without_revision
.strip_prefix("[")
.and_then(|rest| rest.split_once("]"))
.map(|(_, after)| after.trim())
.unwrap_or(without_revision);
Copy link
Copy Markdown
Member

@jieyouxu jieyouxu Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion [TEST-HARNESS-JSON 1/2]: hm I thought about this. This is kinda doing a weird thing of post-processing compiletest-emitted test names, so we introduce an implicit dependency on compiletest test name formatting, which I can't say I'm a huge fan of.

I guess this is not the end of the world...

The caveat is that bootstrap captures test harness JSON output to then post-render the outcome messages. This can be from:

  • libtest JSON when e.g. cargo test is used on say some library crate or such,
  • but it can also come from compiletest-emitted messages which are more "liberal" in how the concept of "test name" is used (that the test name can have a suite prefix or revision suffixes).

If we want to avoid taking on this implicit dependency, we probably will have to introduce separate variants for compiletest-emitted test outcome JSON versus that of libtest, which doesn't really feel that great IMO. That, or maybe we can introduce optional revision / test suite fields for the message variants (which will not be present from other libtest-emitted messages but will be for compiletest-emitted messages).

NB. this was not possible previously, because previously compiletest also shelled out to libtest test executor so compiletest don't have control over the libtest executor emitted libtest JSON message format. But now compiletest uses its own executor and emits emulated libtest JSON messages. So technically I think it might be possible to change the emitted JSON format.

cc @Zalathar (this may be of interest to you)

const MAX_RERUN_PRINTS: usize = 10;

for line in lines {
let trimmed = line.as_str().trim();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion [TEST-HARNESS-JSON 2/2]: I don't feel like hard-blocking this fuctionality on [TEST-HARNESS-JSON 1/2] considerations, but can you please leave a FIXME to consider changing the compiletest-emitted JSON message formats, and maybe change the failed-tests file to have like JSON lines that looks like

{ "suite": "ui", "path": "tests/ui/foo.rs" }
{ "suite": "run-make", "path": "tests/run-make/hello-world/rmake.rs" }

Comment thread src/bootstrap/src/core/build_steps/test/failed_tests.rs Outdated
Comment thread src/bootstrap/src/core/build_steps/test/failed_tests.rs Outdated
// If only `compiler` was passed, do not run this step.
// Instead the `Assemble` step will take care of compiling Rustc.
if run.builder.paths == vec![PathBuf::from("compiler")] {
if run.builder.paths(IsForRerunningTests::DontCare) == vec![PathBuf::from("compiler")] {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion:

The last commit is a little awkward, but I think it's the best way to make it so that we first run all tests that have to be rerun, and then rerun tests passed through the cli.

I really do not like this viral complexity that cross-cuts all these test steps. Can we keep the rerun functionality simple, to only rerun the set of failed tests?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, is it that complex? Cause I do really like doing x test --rerun tests/ui and using that to run a small part (that likely errors) first and then try everything

@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 Apr 6, 2026
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 6, 2026

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@jieyouxu
Copy link
Copy Markdown
Member

@rustbot review

@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 Apr 12, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 13, 2026

☔ The latest upstream changes (presumably #155253) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants