mir-opt tests: run current target on --bless; enable --keep-stage-std #122257

Nadrieril · 2024-03-09T21:31:50Z

Currently, ./x.py test tests/mir-opt runs only the tests for the current target, and ./x.py test tests/mir-opt --bless runs tests for a representative set of targets.

I find this confusing and not what I want: for one I never run tests without --bless because I prefer to look at a git diff. What I however like to do is pass --keep-stage-std when I'm working on the compiler; this doesn't work today with --bless because it tries a bunch of targets for which I don't have a built std.

In this PR, I make it so ./x.py test tests/mir-opt runs the same set of tests regardless of --bless. By default it runs a representative sample of targets, and if --keep-stage-std is set it only runs tests for the current target.

Fixes #122292

rustbot · 2024-03-09T21:31:58Z

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

clubby789 · 2024-03-10T11:36:58Z

@bors r+

bors · 2024-03-10T11:37:01Z

📌 Commit 047e17d has been approved by clubby789

It is now in the queue for this repository.

…lubby789 mir-opt tests: don't run a different set on --bless; enable --keep-stage-std Currently, `./x.py test tests/mir-opt` runs only the tests for the current target, and `./x.py test tests/mir-opt --bless` runs tests for a representative set of targets. I find this confusing and not what I want: for one I never run tests without `--bless` because I prefer to look at a git diff. What I however like to do is pass `--keep-stage-std` when I'm working on the compiler; this doesn't work today with `--bless` because it tries a bunch of targets for which I don't have a built std. In this PR, I make it so `./x.py test tests/mir-opt` runs the same set of tests regardless of `--bless`. By default it runs a representative sample of targets, and if `--keep-stage-std` is set it only runs tests for the current target.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#121573 (unix_sigpipe: Add test for SIGPIPE disposition in child processes) - rust-lang#121754 ([bootstrap] Move the `split-debuginfo` setting to the per-target section) - rust-lang#122205 (ensure that sysroot is properly set for cross-targets) - rust-lang#122257 (mir-opt tests: don't run a different set on --bless; enable --keep-stage-std) - rust-lang#122272 (Subtree update of `rust-analyzer`) Failed merges: - rust-lang#122108 (Add `target.*.runner` configuration for targets) r? `@ghost` `@rustbot` modify labels: rollup

Nadrieril · 2024-03-10T14:21:36Z

Having seen #122292, I'm not sure this is a good idea anymore. For one, in CI we only want to run the test for the current target (I assume that was the original intent of the --bless check). For two, it appears the set of targets chosen here does not cover all the targets. I'll r- until someone can confirm

@bors r-

RalfJung · 2024-03-10T14:25:06Z

In this PR, I make it so ./x.py test tests/mir-opt runs the same set of tests regardless of --bless. By default it runs a representative sample of targets, and if --keep-stage-std is set it only runs tests for the current target.

As you noted, this decreases test coverage. So I don't think it is a good idea.
Also tying this to --keep-stage-std seems at least as surprising as the surprise you expressed in the PR description.

I think it is important that --bless uses a superset of the targets used without, but it doesn't have to be the same set. Can we "just" make sure that the host is always part of the target set for --bless?

Blessing with different targets is needed since MIR differs a lot between panic=abort and panic=unwind. So if you actually want to bless tests then you most likely do want to do it for more than just your host target. It is never useful to bless just the host target.

RalfJung · 2024-03-10T14:29:43Z

src/bootstrap/src/core/build_steps/test.rs

@@ -1568,6 +1568,7 @@ impl Step for MirOpt {
                run(panic_abort_target);
            }
        } else {
+            // If we're keeping a std stage, only run tests for this target.
            run(self.target);


My proposed patch would be to simply move this to before the if.

Alternatively we could enact a policy saying that only-x86_64 is not allowed in mir-opt, and generally making sure that the set of targets in the bless list above is covering all mir-opt tests. That would require tool support however.

It would be nice to have more targets here. At least have all targets that are run in CI

That would take forever though

Nadrieril · 2024-03-10T14:40:25Z

Yeah ok, there are two changes then:

the "representative set of targets" that's used on --bless today is not representative enough. It should indeed include the current target at least;
by nature --keep-stage-std can only work for the current target, so it makes sense to only run the current target when that's on.

Let's keep the --bless switch, I understand its purpose now.

Nadrieril · 2024-03-10T14:58:44Z

Oh, we can keep several stds at once apparently. I was entirely mistaken about --keep-stage-std then.

Nadrieril · 2024-03-10T15:01:03Z

Alright, I'll make a different PR for your fix

RalfJung · 2024-03-10T15:01:36Z

src/bootstrap/src/core/build_steps/test.rs

@@ -1549,6 +1549,8 @@ impl Step for MirOpt {
            })
        };

+        run(self.target);
+
        if builder.config.cmd.bless() {


Suggested change

if builder.config.cmd.bless() {

// Run more targets with `--bless`. But we always run the host target first, since some

// tests use very specific `only` clauses that are not covered by the target set below.

if builder.config.cmd.bless() {

rustbot assigned clubby789 Mar 9, 2024

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 Mar 9, 2024

Nadrieril added the A-testsuite Area: The testsuite used to check the correctness of rustc label Mar 9, 2024

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 Mar 10, 2024

matthiaskrgr mentioned this pull request Mar 10, 2024

Rollup of 5 pull requests #122289

Closed

Nadrieril mentioned this pull request Mar 10, 2024

x.py test mir-opt --bless does not bless some tests #122292

Closed

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 10, 2024

RalfJung reviewed Mar 10, 2024

View reviewed changes

mir-opt: always run tests for the current target

b6ea60f

Nadrieril force-pushed the tweak-mir-opt-tests branch from 047e17d to 1043209 Compare March 10, 2024 14:43

Nadrieril changed the title ~~mir-opt tests: don't run a different set on --bless; enable --keep-stage-std~~ mir-opt tests: run current target on --bless; enable --keep-stage-std Mar 10, 2024

Nadrieril force-pushed the tweak-mir-opt-tests branch from 1043209 to b6ea60f Compare March 10, 2024 14:59

Nadrieril closed this Mar 10, 2024

RalfJung reviewed Mar 10, 2024

View reviewed changes

Nadrieril deleted the tweak-mir-opt-tests branch March 10, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mir-opt tests: run current target on --bless; enable --keep-stage-std #122257

mir-opt tests: run current target on --bless; enable --keep-stage-std #122257

Nadrieril commented Mar 9, 2024 •

edited by RalfJung

Loading

rustbot commented Mar 9, 2024

clubby789 commented Mar 10, 2024

bors commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

RalfJung commented Mar 10, 2024 •

edited

Loading

RalfJung Mar 10, 2024

Nadrieril Mar 10, 2024

Nadrieril Mar 10, 2024

Nadrieril commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

RalfJung Mar 10, 2024

mir-opt tests: run current target on --bless; enable --keep-stage-std #122257

mir-opt tests: run current target on --bless; enable --keep-stage-std #122257

Conversation

Nadrieril commented Mar 9, 2024 • edited by RalfJung Loading

rustbot commented Mar 9, 2024

clubby789 commented Mar 10, 2024

bors commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

RalfJung commented Mar 10, 2024 • edited Loading

RalfJung Mar 10, 2024

Choose a reason for hiding this comment

Nadrieril Mar 10, 2024

Choose a reason for hiding this comment

Nadrieril Mar 10, 2024

Choose a reason for hiding this comment

Nadrieril commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

Nadrieril commented Mar 10, 2024

RalfJung Mar 10, 2024

Choose a reason for hiding this comment

Nadrieril commented Mar 9, 2024 •

edited by RalfJung

Loading

RalfJung commented Mar 10, 2024 •

edited

Loading