Skip to content

compiletest: Fix deadline bugs in new executor #140031

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zalathar
Copy link
Contributor

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

@Zalathar Zalathar added the A-compiletest Area: The compiletest test runner label Apr 19, 2025
@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 Apr 19, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2025

Some changes occurred in src/tools/compiletest

cc @jieyouxu

@jieyouxu
Copy link
Member

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

@Zalathar
Copy link
Contributor Author

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
Copy link
Contributor

Kobzol commented Apr 19, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compiletest Area: The compiletest test runner 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.

4 participants