-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(runner): long synchronous tasks does not time out (fix #2920) #6944
fix(runner): long synchronous tasks does not time out (fix #2920) #6944
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
aedbc6c
to
7bc3c2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find! The change makes sense to me.
Promise.resolve(fn(...args)).then((result) => { | ||
return new Promise(resolve => setTimeout(resolve, 0, result)) | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took a while to understand this but the idea is more or less like this? Executing fn
in sync as usual, but holding off returning until setTimeout(.., 0)
to flush other setTimeout
callbacks. I think code is fine as is, but it's just my commentary.
(async () => {
const result = await fn(...args);
await new Promise(resolve => setTimeout(resolve, 0))
return result;
})()
Oh, well it looks like this code actually breaks Ah I forgot about test.extend
, so I'm not sure I understood this well 😅args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! Yes that's the same thing, I can try refactor with your code if it's best.
The Browser: webkit test was passing in one of my previous run: https://github.com/vitest-dev/vitest/actions/runs/11955759654 Is it a flakiness issue ? It might just need a job re-run to pass. [Edit]: Just re-pushed an identical commit to trigger the re-run. Test are passing now ✅ |
7bc3c2a
to
3414830
Compare
Description
Fixes #2920
This PR solves Vitest ignoring timeouts on long synchronous tasks, that are blocking until the task and the timeout are settled simultaneously.
In this case we want to give priority to the timeout, because a test running longer than it's own timeout shall fail.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.