-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred in src/tools/compiletest cc @jieyouxu |
I'll do some local testing of this on Monday (I'm out tmrw) unless someone beats me to it. |
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. |
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()
} |
Some manual benchmark results from my machine after these changes:
(These are a bit quicker than my measurements in #139998, because I got rid of some background/indexing processes.) |
Mocking 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. |
The experimental new executor for compiletest (#139660) was found to have two major bugs in deadline handling for detecting slow tests:
now
and test deadlines was reversed, causing no timeouts to ever be recognised.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 concretempsc::Receiver
APIs (in addition toInstant::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