feat: add advanceTimers option
#907
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit c2375eb:
|
src/options.ts
Outdated
| * | ||
| * @example jest.advanceTimersByTime | ||
| */ | ||
| advanceTimers?: (delay: number) => void |
There was a problem hiding this comment.
I think this should return Promise<void>. The resulting await on this enables the user to let third-party code run after the timers were advanced and before we continue interacting. Otherwise asynchronous code during the timeout might not be executed until after the next interaction which might result in hard to debug issues.
There was a problem hiding this comment.
The place where this advanceTimers function would be used is already inside of a promise and resolving the promise is already from a setTimeout. Can you give an example of some code which would need advanceTimers to return a promise?
There was a problem hiding this comment.
function wait(t: number) {
return new Promise<void>(r => setTimeout(r, t))
}
function waitVoidAdvance(t: number, advance: (t: number) => void) {
return new Promise<void>(resolve => {
setTimeout(() => resolve(), t)
advance(t)
})
}
function waitAwaitAdvance(t: number, advance: (t: number) => Promise<unknown> | unknown) {
return Promise.all([
new Promise<void>(resolve => setTimeout(() => resolve(), t)),
advance(t)
])
}
async function impl(
log: string[],
waitImpl: (t: number) => Promise<unknown> | unknown
) {
// event handler
void (async () => {
log.push('event')
await wait(0)
log.push('then')
void wait(20).then(() => {
log.push('after 20')
})
await wait(0)
log.push('next')
await wait(0)
log.push('x')
await wait(0)
log.push('y')
await wait(0)
log.push('z')
await wait(100)
log.push('delayed')
})()
// delay
await waitImpl(50)
log.push('continue')
}
describe.each([false, true])('usePromise: %s', (useAwaitAdvance) => {
test.each([false, true])('fakeTimers: %s', async (useFakeTimers) => {
const log: string[] = []
if (useFakeTimers) {
jest.useFakeTimers()
}
await impl(
log,
useAwaitAdvance
? (t) => waitAwaitAdvance(
t,
useFakeTimers
? async (u: number) => {
do {
jest.advanceTimersByTime(Math.min(u, 1))
for(let before;;) {
before = jest.getTimerCount()
await new Promise<void>(r => r())
if (jest.getTimerCount() === before) {
break
}
jest.advanceTimersByTime(0)
}
} while(--u > 0)
}
: () => Promise.resolve()
)
: (t) => waitVoidAdvance(
t,
useFakeTimers
? jest.advanceTimersByTime
: () => undefined
),
)
jest.useRealTimers()
await wait(10)
expect(log).toEqual([
'event',
'then',
'next',
'x',
'y',
'z',
'after 20',
'continue',
])
})
})There was a problem hiding this comment.
Have you ever seen code like that in a real project? That's gnarly!
In the wait function, the promise that advanceTimers returns is not awaited though (nor would it make sense to be). Which is confusing to a user who passes a promise since if they pass:
async (delay: number) => {
jest.advanceTimersByTime(delay);
await new Promise((resolve) => /* do some additional work */);
}
It would appear as if the timers would wait additional time for that work to be completed before continuing. But because the advanceTimersByTime call happens first, then it wouldn't wait and the rest of the test would continue while the promise was still pending.
I think it'd be best to avoid a footgun like this. In the event that someone does want to write a crazy function here, and wants to use await, they can always do:
(delay: number) => {
(async () => {
/* do your crazy async work */
})();
}
Which has the benefit of being clear that the caller (this library) doesn't care about your promise.
There was a problem hiding this comment.
The code above just demonstrates the problem in a reproducible manner.
In real-world scenarios this usually isn't that obvious, the pushes to the event loop can be implemented in nested/imported modules and/or can involve event-driven APIs.
(delay: number) => { (async () => { /* do your crazy async work */ })(); }
^ This would not allow the asynchronous code after any await to run before we continue with the next interaction that is supposed to happen "later".
The problem is that fake timers create a situation that normally doesn't exist: Microtasks being queued and not executed before picking the next "macrotask". This is because the next "macrotask" is actually queued synchronously while the microtask still is queued through the event loop.
There was a problem hiding this comment.
Got it. I don't understand why anyone would ever do this. But sure, I can make it support promises too if someone wants to do this.
Codecov Report
@@ Coverage Diff @@
## main #907 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 85 85
Lines 1778 1782 +4
Branches 641 641
=========================================
+ Hits 1778 1782 +4
Continue to review full report at Codecov.
|
|
@ph-fritsche let me know if there are any other changes you'd like to see. I think this is ready to go though! :) |
|
This looks like a nice addition 👏 . I wonder if the name shouldn't be more general. I can't think of another use case besides advancing timers but who knows. I think calling it |
|
@stokesman I'd totally agree with you if there were ideas on how else it could be used. But I'd rather not rename something to be more ambiguous (and possibly confusing) because of some hypothetical future that may not exist. 😄 If you can think of other places in the library where a similar, abstract, version of this pattern could be used, let's do it! |
|
@all-contributors add @CreativeTechGuy code |
|
I've put up a pull request to add @CreativeTechGuy! 🎉 |
|
🎉 This PR is included in version 14.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Implement an
advanceTimersoption from #585 and the second half of this commentWhy:
The internal delays in the library don't play nice with fake timers in testing frameworks
How:
I'm not really sure how to answer this question. With code? haha
Checklist: