Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add advanceTimers option #907

Merged
merged 4 commits into from
Apr 11, 2022
Merged

feat: add advanceTimers option #907

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
16b3585
advance timers option
CreativeTechGuy Apr 5, 2022
a54dc2c
refactor wait to accept config object
CreativeTechGuy Apr 5, 2022
0ac4ef6
support advanceTimers returning a promise
CreativeTechGuy Apr 7, 2022
c2375eb
Merge branch 'main' into pr/advance-timers
ph-fritsche Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/keyboard/keyboardAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export async function keyboardAction(
await keyboardKeyAction(config, actions[i])

if (typeof config.delay === 'number' && i < actions.length - 1) {
await wait(config.delay)
await wait(config.delay, config.advanceTimers)
}
}
}
Expand All @@ -32,7 +32,7 @@ async function keyboardKeyAction(
config: Config,
{keyDef, releasePrevious, releaseSelf, repeat}: KeyboardAction,
) {
const {document, keyboardState, delay} = config
const {document, keyboardState, delay, advanceTimers} = config
const getCurrentElement = () => getActive(document)

// Release the key automatically if it was pressed before.
Expand All @@ -51,7 +51,7 @@ async function keyboardKeyAction(
}

if (typeof delay === 'number' && i < repeat) {
await wait(delay)
await wait(delay, advanceTimers)
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,13 @@ export interface Options {
* Defaults to `true` when calling the APIs per `setup`.
*/
writeToClipboard?: boolean

/**
* A function to be called internally to advance your fake timers (if applicable)
*
* @example jest.advanceTimersByTime
*/
advanceTimers?: (delay: number) => void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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',
        ])
    })

})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand All @@ -137,6 +144,7 @@ export const defaultOptionsDirect: Required<Options> = {
skipClick: false,
skipHover: false,
writeToClipboard: false,
advanceTimers: () => void 0,
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/pointer/pointerAction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export async function pointerAction(config: Config, actions: PointerAction[]) {

if (typeof config.delay === 'number') {
if (i < actions.length - 1) {
await wait(config.delay)
await wait(config.delay, config.advanceTimers)
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/utils/misc/wait.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
export function wait(time?: number) {
return new Promise<void>(resolve => setTimeout(() => resolve(), time))
import type {Options} from '../../options'

export function wait(
time: number,
advanceTimers: Exclude<Options['advanceTimers'], undefined>,
) {
CreativeTechGuy marked this conversation as resolved.
Show resolved Hide resolved
return new Promise<void>(resolve => {
setTimeout(() => resolve(), time)
advanceTimers(time)
})
}
9 changes: 9 additions & 0 deletions tests/utils/misc/wait.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import {wait} from '#src/utils/misc/wait'

test('advances timers when set', async () => {
jest.useFakeTimers()
jest.setTimeout(50)
// If this wasn't advancing fake timers, we'd timeout and fail the test
await wait(10000, jest.advanceTimersByTime)
jest.useRealTimers()
})