compiletest: Fix deadline bugs in new executor #140031

Zalathar · 2025-04-19T07:54:20Z

The experimental new executor for compiletest (#139660) was found to have two major bugs in deadline handling for detecting slow tests:

The comparison between now and test deadlines was reversed, causing no timeouts to ever be recognised.
After fixing that bug, it was found that the existing code would issue timeouts for any test that had started more than 60 seconds ago, even if the test had finished long before its deadline was reached.

This PR fixes those bugs.

(The new executor is not yet enabled by default, so this PR has no immediate effect on contributors.)

I noted in #139998 (comment) that I hoped to have some unit tests to accompany these fixes. Unfortunately that turned out to be infeasible, because DeadlineQueue is tightly coupled to concrete mpsc::Receiver APIs (in addition to Instant::now), and trying to mock all of those would make the code much more complicated.

I did, however, add a few assertions that would have caught the failure to remove tests from the queue after their deadline.

r? jieyouxu

rustbot · 2025-04-19T07:54:26Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

jieyouxu · 2025-04-19T07:59:04Z

I'll do some local testing of this on Monday (I'm out tmrw) unless someone beats me to it.

Zalathar · 2025-04-19T08:02:38Z

For manual testing, I suggest changing the (hardcoded) timeout to 5 seconds, as there are several tests in the ui suite that will naturally take longer than that.

Kobzol · 2025-04-19T09:33:35Z

FWIW, you can test Instant::now() pretty easily even without dependency injection, by doing something like this:

#[cfg(test)]
fn now() -> Instant { return global or thread-local variable set in tests }

#[cfg(not(test))]
fn now() -> Instant {
    Instant::now()
}

Zalathar · 2025-04-19T10:15:36Z

Some manual benchmark results from my machine after these changes:

$ x test ui -- --force-rerun
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 162.24s
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 163.28s
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 161.84s

$ x test ui -- --force-rerun -n
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 162.32s
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 163.64s
test result: ok. 18610 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 162.41s

(These are a bit quicker than my measurements in #139998, because I got rid of some background/indexing processes.)

Zalathar · 2025-04-19T11:05:58Z

FWIW, you can test Instant::now() pretty easily even without dependency injection, by doing something like this:

Mocking Instant::now is not so bad, but after doing that I found that I still needed to find ways to mock things like mpsc::Receiver::recv_timeout and CollectedTest, which is harder. In my draft, it got to the point where DeadlineQueue contained more test scaffolding than code.

Maybe there's another design that makes testing easier, but I was reluctant to get too deep in the weeds of redesigning things to be fundamentally different from how libtest does it.

jieyouxu · 2025-04-21T11:24:52Z

Haven't quite gotten to this PR today, I'll do the testing tmrw morning.

jieyouxu

Thanks! I exercised this locally on x86_64-unknown-linux-gnu and x86_64-pc-windows-msvc, and I am no longer observing the all-running-tests-produce-slow-warning behavior. Tested with a synthetic long-running foo.rs and changing deadline to 5s.

One style nit, but otherwise LGTM

src/tools/compiletest/src/executor/deadline.rs

jieyouxu · 2025-04-22T10:04:39Z

@rustbot author

rustbot · 2025-04-22T10:04:43Z

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

Zalathar · 2025-04-22T12:04:00Z

Rebased and added note to DeadlineQueue::now (diff).

@rustbot ready

jieyouxu · 2025-04-22T13:43:31Z

Thanks!
@bors r+ rollup

bors · 2025-04-22T13:43:34Z

📌 Commit f7b1e03 has been approved by jieyouxu

It is now in the queue for this repository.

…enton Rollup of 8 pull requests Successful merges: - rust-lang#139617 (Use posix_spawn on cygwin) - rust-lang#139921 (improve diagnostic for raw pointer field access with ->) - rust-lang#140031 (compiletest: Fix deadline bugs in new executor) - rust-lang#140072 (handle function alignment in miri) - rust-lang#140104 (Fix auto diff failing on inherent impl blocks) - rust-lang#140124 (Update books) - rust-lang#140144 (Handle another negated literal in `eat_token_lit`.) - rust-lang#140149 (test_nan: ensure the NAN contant is quiet) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#140031 - Zalathar:deadline, r=jieyouxu compiletest: Fix deadline bugs in new executor The experimental new executor for compiletest (rust-lang#139660) was found to have two major bugs in deadline handling for detecting slow tests: - The comparison between `now` and test deadlines was reversed, causing no timeouts to ever be recognised. - After fixing that bug, it was found that the existing code would issue timeouts for any test that had started more than 60 seconds ago, even if the test had finished long before its deadline was reached. This PR fixes those bugs. (The new executor is not yet enabled by default, so this PR has no immediate effect on contributors.) --- I noted in rust-lang#139998 (comment) that I hoped to have some unit tests to accompany these fixes. Unfortunately that turned out to be infeasible, because `DeadlineQueue` is tightly coupled to concrete `mpsc::Receiver` APIs (in addition to `Instant::now`), and trying to mock all of those would make the code much more complicated. I did, however, add a few assertions that would have caught the failure to remove tests from the queue after their deadline. r? jieyouxu

Zalathar added the A-compiletest Area: The compiletest test runner label Apr 19, 2025

rustbot assigned jieyouxu Apr 19, 2025

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 Apr 19, 2025

Zalathar mentioned this pull request Apr 19, 2025

compiletest: Use the new non-libtest executor by default #139998

Merged

jieyouxu approved these changes Apr 22, 2025

View reviewed changes

src/tools/compiletest/src/executor/deadline.rs Show resolved Hide resolved

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 22, 2025

Zalathar force-pushed the deadline branch from bbab72b to 2f7f96c Compare April 22, 2025 11:55

compiletest: Fix deadline bugs in new executor

f7b1e03

Zalathar force-pushed the deadline branch from 2f7f96c to f7b1e03 Compare April 22, 2025 12:03

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 22, 2025

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 Apr 22, 2025

ChrisDenton mentioned this pull request Apr 22, 2025

Rollup of 8 pull requests #140165

Merged

bors merged commit c046a43 into rust-lang:master Apr 23, 2025
6 checks passed

rustbot added this to the 1.88.0 milestone Apr 23, 2025

Zalathar deleted the deadline branch April 23, 2025 01:22

jieyouxu mentioned this pull request Apr 23, 2025

Tracking issue for new compiletest executor #140192

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiletest: Fix deadline bugs in new executor #140031

compiletest: Fix deadline bugs in new executor #140031

Zalathar commented Apr 19, 2025

rustbot commented Apr 19, 2025

jieyouxu commented Apr 19, 2025

Zalathar commented Apr 19, 2025

Kobzol commented Apr 19, 2025

Zalathar commented Apr 19, 2025

Zalathar commented Apr 19, 2025

jieyouxu commented Apr 21, 2025

jieyouxu left a comment •

edited

Loading

jieyouxu commented Apr 22, 2025

rustbot commented Apr 22, 2025

Zalathar commented Apr 22, 2025

jieyouxu commented Apr 22, 2025

bors commented Apr 22, 2025

compiletest: Fix deadline bugs in new executor #140031

compiletest: Fix deadline bugs in new executor #140031

Conversation

Zalathar commented Apr 19, 2025

rustbot commented Apr 19, 2025

jieyouxu commented Apr 19, 2025

Zalathar commented Apr 19, 2025

Kobzol commented Apr 19, 2025

Zalathar commented Apr 19, 2025

Zalathar commented Apr 19, 2025

jieyouxu commented Apr 21, 2025

jieyouxu left a comment • edited Loading

Choose a reason for hiding this comment

jieyouxu commented Apr 22, 2025

rustbot commented Apr 22, 2025

Zalathar commented Apr 22, 2025

jieyouxu commented Apr 22, 2025

bors commented Apr 22, 2025

jieyouxu left a comment •

edited

Loading