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

Print a helpful message if any tests were skipped for being up-to-date #130131

Merged
merged 2 commits into from
Sep 9, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Sep 9, 2024

When running tests without the --force-rerun flag, compiletest will automatically skip any tests that (in its judgement) don't need to be run again since the last time they were run.

This is normally very useful, but can occasionally be confusing, especially in edge-cases where up-to-date checking is not completely accurate (or the test is flaky).

This PR makes bootstrap count the number of tests that were ignored for being up-to-date (via a hard-coded check on the ignore reason), and prints a helpful message when that number is nonzero.


Sample output:

test result: ok. 4 passed; 0 failed; 17578 ignored; 0 measured; 0 filtered out; finished in 463.79ms

help: ignored 17295 up-to-date tests; use `--force-rerun` to prevent this

Build completed successfully in 0:00:07

When running tests without the `--force-rerun` flag, compiletest will
automatically skip any tests that (in its judgement) don't need to be run again
since the last time they were run.

This patch adds an explicit reason to those skipped tests, which is visible
when running with `rust.verbose-tests = true` in `config.toml`.
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2024

r? @Kobzol

rustbot has assigned @Kobzol.
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-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 Sep 9, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 9, 2024

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 9, 2024

I'm hoping that this is unobtrusive enough to be worth always emitting whenever up-to-date tests are skipped, but I'm certainly open to other opinions.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

This is a good improvement, but I wonder if we could even make up-to-date tests a first class citizen, so that we could print something like this:

test result: ok. 0 passed; 0 failed; 0 ignored; 1 up-to-date; 0 measured; 17581 filtered out; finished in 3.86ms

and to make the detection more robust (rather than checking a string in the ignore reason. I find it weird to treat up-to-date tests as ignored, it can also be misleading, e.g. if we check ignored tests on CI, if for some reason some of them were just up-to-date due to some unexpected bootstrap behavior, then we would miscount them. And in fact detecting that CI has no up-to-date tests could also be useful.

@jieyouxu what do you think?

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 9, 2024

In terms of proper first-class support for up-to-date tests, we're somewhat constrained by the fact that the test results seen by bootstrap travel through libtest as an intermediate step, so we would need to be able to propagate that extra information through libtest.

I'm not sure what's involved in adding more custom unstable stuff to libtest.

@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 9, 2024

That said, I would certainly be happy to see better reporting of up-to-date tests in some form.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

Oh, I didn't realize that it's from libtest, rather than compiletest. Yeah, then probably let's not go there for now 😆

@bors r+

@bors
Copy link
Contributor

bors commented Sep 9, 2024

📌 Commit acccb39 has been approved by Kobzol

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 Sep 9, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Sep 9, 2024

IIRC the actual status line is printed by bootstrap, based on stats reported by libtest.

So we could do some arithmetic and break out the up-to-date tests from the overall ignored count, but I’m a bit wary of adding that much magic to the main status line (rather than faithfully reporting the libtest numbers).

@Kobzol
Copy link
Contributor

Kobzol commented Sep 9, 2024

Aah, I was looking for the line in compiletest instead of bootstrap 🤦 It also looks like the actual libtest output line, so I didn't realize that bootstrap emulates it.

I don't think that we necessarily need to reproduce libtest output 1:1, but good point. Let's land your change, I think it's unobtrusive enough, and then we can discuss with others if they would be ok with making larger changes.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 9, 2024
Print a helpful message if any tests were skipped for being up-to-date

When running tests without the `--force-rerun` flag, compiletest will automatically skip any tests that (in its judgement) don't need to be run again since the last time they were run.

This is normally very useful, but can occasionally be confusing, especially in edge-cases where up-to-date checking is not completely accurate (or the test is flaky).

This PR makes bootstrap count the number of tests that were ignored for being up-to-date (via a hard-coded check on the ignore reason), and prints a helpful message when that number is nonzero.

---

Sample output:

```text
test result: ok. 4 passed; 0 failed; 17578 ignored; 0 measured; 0 filtered out; finished in 463.79ms

help: ignored 17295 up-to-date tests; use `--force-rerun` to prevent this

Build completed successfully in 0:00:07
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129929 (`rustc_mir_transform` cleanups, round 2)
 - rust-lang#130022 (Dataflow/borrowck lifetime cleanups)
 - rust-lang#130064 (fix ICE in CMSE type validation)
 - rust-lang#130067 (Remove redundant check in `symlink_hard_link` test)
 - rust-lang#130131 (Print a helpful message if any tests were skipped for being up-to-date)
 - rust-lang#130137 (Fix ICE caused by missing span in a region error)
 - rust-lang#130153 (use verbose flag as a default value for `rust.verbose-tests`)
 - rust-lang#130154 (Stabilize `char::MIN`)
 - rust-lang#130158 (Update books)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3658bfb into rust-lang:master Sep 9, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2024
Rollup merge of rust-lang#130131 - Zalathar:up-to-date, r=Kobzol

Print a helpful message if any tests were skipped for being up-to-date

When running tests without the `--force-rerun` flag, compiletest will automatically skip any tests that (in its judgement) don't need to be run again since the last time they were run.

This is normally very useful, but can occasionally be confusing, especially in edge-cases where up-to-date checking is not completely accurate (or the test is flaky).

This PR makes bootstrap count the number of tests that were ignored for being up-to-date (via a hard-coded check on the ignore reason), and prints a helpful message when that number is nonzero.

---

Sample output:

```text
test result: ok. 4 passed; 0 failed; 17578 ignored; 0 measured; 0 filtered out; finished in 463.79ms

help: ignored 17295 up-to-date tests; use `--force-rerun` to prevent this

Build completed successfully in 0:00:07
```
@Zalathar Zalathar deleted the up-to-date branch September 9, 2024 21:48
@jieyouxu
Copy link
Member

Yeah this seems fine, I'm less concerned about up-to-date tests being skipped (which is intuitive to me at least), but more concerned about tests being incorrectly skipped (lol)

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-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
Development

Successfully merging this pull request may close these issues.

5 participants