From 61808601c646f38b1c68ef8ff0f0fcaf6d918207 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 13:06:01 +0200 Subject: [PATCH 1/7] Use same interval handler in all supported clocks --- src/__tests__/wait-for.js | 4 +-- src/wait-for.js | 55 +++++++++++++++++++++------------------ 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index a41a7360..52d17ada 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -207,7 +207,7 @@ test('if you switch from fake timers to real timers during the wait period you g const error = await waitForError expect(error.message).toMatchInlineSnapshot( - `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to real timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) // stack trace has this file in it expect(error.stack).toMatch(__dirname) @@ -223,7 +223,7 @@ test('if you switch from real timers to fake timers during the wait period you g const error = await waitForError expect(error.message).toMatchInlineSnapshot( - `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to fake timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) // stack trace has this file in it expect(error.stack).toMatch(__dirname) diff --git a/src/wait-for.js b/src/wait-for.js index 0a1e75ed..bf573f39 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -43,14 +43,15 @@ function waitFor( } return new Promise(async (resolve, reject) => { - let lastError, intervalId, observer + let lastError, observer let finished = false let promiseStatus = 'idle' - const overallTimeoutTimer = setTimeout(handleTimeout, timeout) + const overallTimeout = setTimeout(handleTimeout, timeout) + const intervalId = setInterval(handleInterval, interval) - const usingJestFakeTimers = jestFakeTimersAreEnabled() - if (usingJestFakeTimers) { + const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() + if (wasUsingJestFakeTimers) { checkCallback() // this is a dangerous rule to disable because it could lead to an // infinite loop. However, eslint isn't smart enough to know that we're @@ -58,11 +59,10 @@ function waitFor( // waiting or when we've timed out. // eslint-disable-next-line no-unmodified-loop-condition while (!finished) { - if (!jestFakeTimersAreEnabled()) { - const error = new Error( - `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to real timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, - ) - if (!showOriginalStackTrace) copyStackTrace(error, stackTraceError) + try { + // jest.advanceTimersByTime will not throw if + ensureInvariantTimers() + } catch (error) { reject(error) return } @@ -73,12 +73,6 @@ function waitFor( // by an interval instead. (We have a test for this case). jest.advanceTimersByTime(interval) - // It's really important that checkCallback is run *before* we flush - // in-flight promises. To be honest, I'm not sure why, and I can't quite - // think of a way to reproduce the problem in a test, but I spent - // an entire day banging my head against a wall on this. - checkCallback() - // In this rare case, we *need* to wait for in-flight promises // to resolve before continuing. We don't need to take advantage // of parallelization so we're fine. @@ -96,19 +90,18 @@ function waitFor( reject(e) return } - intervalId = setInterval(checkRealTimersCallback, interval) const {MutationObserver} = getWindowFromNode(container) - observer = new MutationObserver(checkRealTimersCallback) + observer = new MutationObserver(handleInterval) observer.observe(container, mutationObserverOptions) checkCallback() } function onDone(error, result) { finished = true - clearTimeout(overallTimeoutTimer) + clearTimeout(overallTimeout) + clearInterval(intervalId) - if (!usingJestFakeTimers) { - clearInterval(intervalId) + if (observer !== undefined) { observer.disconnect() } @@ -119,15 +112,27 @@ function waitFor( } } - function checkRealTimersCallback() { - if (jestFakeTimersAreEnabled()) { + function ensureInvariantTimers() { + const isUsingJestFakeTimers = jestFakeTimersAreEnabled() + if (wasUsingJestFakeTimers !== isUsingJestFakeTimers) { const error = new Error( - `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to fake timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using ${ + wasUsingJestFakeTimers ? 'fake timers' : 'real timers' + } to ${ + isUsingJestFakeTimers ? 'fake timers' : 'real timers' + } while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) if (!showOriginalStackTrace) copyStackTrace(error, stackTraceError) - return reject(error) - } else { + throw error + } + } + + function handleInterval() { + try { + ensureInvariantTimers() return checkCallback() + } catch (error) { + return reject(error) } } From cacbb9f70e7f794648339048f723e9932c8bedfb Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 13:07:35 +0200 Subject: [PATCH 2/7] invariant initial checkCallback --- src/wait-for.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/wait-for.js b/src/wait-for.js index bf573f39..dcae5255 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -49,10 +49,10 @@ function waitFor( const overallTimeout = setTimeout(handleTimeout, timeout) const intervalId = setInterval(handleInterval, interval) + checkCallback() const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() if (wasUsingJestFakeTimers) { - checkCallback() // this is a dangerous rule to disable because it could lead to an // infinite loop. However, eslint isn't smart enough to know that we're // setting finished inside `onDone` which will be called when we're done @@ -93,7 +93,6 @@ function waitFor( const {MutationObserver} = getWindowFromNode(container) observer = new MutationObserver(handleInterval) observer.observe(container, mutationObserverOptions) - checkCallback() } function onDone(error, result) { From 34fc17a2be0e3463f67b1fd27daa78b3be1386f0 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 13:09:07 +0200 Subject: [PATCH 3/7] Use MutationObserver in all clocks Makes for a more consistent elapsed times in fake timers --- src/wait-for.js | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/wait-for.js b/src/wait-for.js index dcae5255..cd1320ba 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -43,12 +43,23 @@ function waitFor( } return new Promise(async (resolve, reject) => { - let lastError, observer + let lastError let finished = false let promiseStatus = 'idle' const overallTimeout = setTimeout(handleTimeout, timeout) const intervalId = setInterval(handleInterval, interval) + + try { + checkContainerType(container) + } catch (e) { + reject(e) + return + } + const {MutationObserver} = getWindowFromNode(container) + const observer = new MutationObserver(handleInterval) + observer.observe(container, mutationObserverOptions) + checkCallback() const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() @@ -83,16 +94,6 @@ function waitFor( jest.advanceTimersByTime(0) }) } - } else { - try { - checkContainerType(container) - } catch (e) { - reject(e) - return - } - const {MutationObserver} = getWindowFromNode(container) - observer = new MutationObserver(handleInterval) - observer.observe(container, mutationObserverOptions) } function onDone(error, result) { @@ -100,9 +101,7 @@ function waitFor( clearTimeout(overallTimeout) clearInterval(intervalId) - if (observer !== undefined) { - observer.disconnect() - } + observer.disconnect() if (error) { reject(error) From 193c8b45df2587770843cd6801e92491123478cd Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 13:45:15 +0200 Subject: [PATCH 4/7] feat(config): Add advanceTimersWrapper --- src/config.ts | 4 +++- src/wait-for.js | 39 +++++++++++++++++++++++---------------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/src/config.ts b/src/config.ts index 58c4b141..9b3e4446 100644 --- a/src/config.ts +++ b/src/config.ts @@ -4,6 +4,7 @@ import {prettyDOM} from './pretty-dom' type Callback = () => T interface InternalConfig extends Config { _disableExpensiveErrorDiagnostics: boolean + advanceTimersWrapper(cb: (...args: unknown[]) => unknown): unknown } // It would be cleaner for this to live inside './queries', but @@ -12,7 +13,7 @@ interface InternalConfig extends Config { let config: InternalConfig = { testIdAttribute: 'data-testid', asyncUtilTimeout: 1000, - // this is to support React's async `act` function. + // asyncWrapper and advanceTimersWrapper is to support React's async `act` function. // forcing react-testing-library to wrap all async functions would've been // a total nightmare (consider wrapping every findBy* query and then also // updating `within` so those would be wrapped too. Total nightmare). @@ -20,6 +21,7 @@ let config: InternalConfig = { // react-testing-library to use. For that reason, this feature will remain // undocumented. asyncWrapper: cb => cb(), + advanceTimersWrapper: cb => cb(), eventWrapper: cb => cb(), // default value for the `hidden` option in `ByRole` queries defaultHidden: false, diff --git a/src/wait-for.js b/src/wait-for.js index cd1320ba..1ba9c6da 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -64,6 +64,8 @@ function waitFor( const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() if (wasUsingJestFakeTimers) { + const {advanceTimersWrapper} = getConfig() + // this is a dangerous rule to disable because it could lead to an // infinite loop. However, eslint isn't smart enough to know that we're // setting finished inside `onDone` which will be called when we're done @@ -73,26 +75,31 @@ function waitFor( try { // jest.advanceTimersByTime will not throw if ensureInvariantTimers() + + advanceTimersWrapper(() => { + // we *could* (maybe should?) use `advanceTimersToNextTimer` but it's + // possible that could make this loop go on forever if someone is using + // third party code that's setting up recursive timers so rapidly that + // the user's timer's don't get a chance to resolve. So we'll advance + // by an interval instead. (We have a test for this case). + jest.advanceTimersByTime(interval) + }) + + // In this rare case, we *need* to wait for in-flight promises + // to resolve before continuing. We don't need to take advantage + // of parallelization so we're fine. + // https://stackoverflow.com/a/59243586/971592 + // eslint-disable-next-line no-await-in-loop + await advanceTimersWrapper(async () => { + await new Promise(r => { + setTimeout(r, 0) + jest.advanceTimersByTime(0) + }) + }) } catch (error) { reject(error) return } - // we *could* (maybe should?) use `advanceTimersToNextTimer` but it's - // possible that could make this loop go on forever if someone is using - // third party code that's setting up recursive timers so rapidly that - // the user's timer's don't get a chance to resolve. So we'll advance - // by an interval instead. (We have a test for this case). - jest.advanceTimersByTime(interval) - - // In this rare case, we *need* to wait for in-flight promises - // to resolve before continuing. We don't need to take advantage - // of parallelization so we're fine. - // https://stackoverflow.com/a/59243586/971592 - // eslint-disable-next-line no-await-in-loop - await new Promise(r => { - setTimeout(r, 0) - jest.advanceTimersByTime(0) - }) } } From d033b554bfcbd5716dadd3a20eab94ab66c19f98 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 14:09:39 +0200 Subject: [PATCH 5/7] poke csb From ce51eecf24d7cc468b55c14867589f18f765ee3a Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 14:21:45 +0200 Subject: [PATCH 6/7] advanceTimersWrapper -> unstable_advanceTimersWrapper --- src/config.ts | 3 +-- src/wait-for.js | 2 +- types/config.d.ts | 5 +++++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/config.ts b/src/config.ts index 9b3e4446..47762b18 100644 --- a/src/config.ts +++ b/src/config.ts @@ -4,7 +4,6 @@ import {prettyDOM} from './pretty-dom' type Callback = () => T interface InternalConfig extends Config { _disableExpensiveErrorDiagnostics: boolean - advanceTimersWrapper(cb: (...args: unknown[]) => unknown): unknown } // It would be cleaner for this to live inside './queries', but @@ -21,7 +20,7 @@ let config: InternalConfig = { // react-testing-library to use. For that reason, this feature will remain // undocumented. asyncWrapper: cb => cb(), - advanceTimersWrapper: cb => cb(), + unstable_advanceTimersWrapper: cb => cb(), eventWrapper: cb => cb(), // default value for the `hidden` option in `ByRole` queries defaultHidden: false, diff --git a/src/wait-for.js b/src/wait-for.js index 1ba9c6da..13518721 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -64,7 +64,7 @@ function waitFor( const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() if (wasUsingJestFakeTimers) { - const {advanceTimersWrapper} = getConfig() + const {unstable_advanceTimersWrapper: advanceTimersWrapper} = getConfig() // this is a dangerous rule to disable because it could lead to an // infinite loop. However, eslint isn't smart enough to know that we're diff --git a/types/config.d.ts b/types/config.d.ts index c9c33633..6a3d1247 100644 --- a/types/config.d.ts +++ b/types/config.d.ts @@ -1,5 +1,10 @@ export interface Config { testIdAttribute: string + /** + * WARNING: `unstable` prefix means this API may change in patch and minor releases. + * @param cb + */ + unstable_advanceTimersWrapper(cb: (...args: unknown[]) => unknown): unknown // eslint-disable-next-line @typescript-eslint/no-explicit-any asyncWrapper(cb: (...args: any[]) => any): Promise // eslint-disable-next-line @typescript-eslint/no-explicit-any From efd21b4ab85a2140dd8b36b5c8e7832dc320e4e9 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Sun, 5 Sep 2021 14:24:56 +0200 Subject: [PATCH 7/7] Defer refactoring to later --- src/__tests__/wait-for.js | 4 +- src/wait-for.js | 121 ++++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 65 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index 52d17ada..a41a7360 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -207,7 +207,7 @@ test('if you switch from fake timers to real timers during the wait period you g const error = await waitForError expect(error.message).toMatchInlineSnapshot( - `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to real timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) // stack trace has this file in it expect(error.stack).toMatch(__dirname) @@ -223,7 +223,7 @@ test('if you switch from real timers to fake timers during the wait period you g const error = await waitForError expect(error.message).toMatchInlineSnapshot( - `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to fake timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) // stack trace has this file in it expect(error.stack).toMatch(__dirname) diff --git a/src/wait-for.js b/src/wait-for.js index 13518721..774d8a85 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -43,72 +43,79 @@ function waitFor( } return new Promise(async (resolve, reject) => { - let lastError + let lastError, intervalId, observer let finished = false let promiseStatus = 'idle' - const overallTimeout = setTimeout(handleTimeout, timeout) - const intervalId = setInterval(handleInterval, interval) + const overallTimeoutTimer = setTimeout(handleTimeout, timeout) - try { - checkContainerType(container) - } catch (e) { - reject(e) - return - } - const {MutationObserver} = getWindowFromNode(container) - const observer = new MutationObserver(handleInterval) - observer.observe(container, mutationObserverOptions) - - checkCallback() - - const wasUsingJestFakeTimers = jestFakeTimersAreEnabled() - if (wasUsingJestFakeTimers) { + const usingJestFakeTimers = jestFakeTimersAreEnabled() + if (usingJestFakeTimers) { const {unstable_advanceTimersWrapper: advanceTimersWrapper} = getConfig() - + checkCallback() // this is a dangerous rule to disable because it could lead to an // infinite loop. However, eslint isn't smart enough to know that we're // setting finished inside `onDone` which will be called when we're done // waiting or when we've timed out. // eslint-disable-next-line no-unmodified-loop-condition while (!finished) { - try { - // jest.advanceTimersByTime will not throw if - ensureInvariantTimers() - - advanceTimersWrapper(() => { - // we *could* (maybe should?) use `advanceTimersToNextTimer` but it's - // possible that could make this loop go on forever if someone is using - // third party code that's setting up recursive timers so rapidly that - // the user's timer's don't get a chance to resolve. So we'll advance - // by an interval instead. (We have a test for this case). - jest.advanceTimersByTime(interval) - }) - - // In this rare case, we *need* to wait for in-flight promises - // to resolve before continuing. We don't need to take advantage - // of parallelization so we're fine. - // https://stackoverflow.com/a/59243586/971592 - // eslint-disable-next-line no-await-in-loop - await advanceTimersWrapper(async () => { - await new Promise(r => { - setTimeout(r, 0) - jest.advanceTimersByTime(0) - }) - }) - } catch (error) { + if (!jestFakeTimersAreEnabled()) { + const error = new Error( + `Changed from using fake timers to real timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to real timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + ) + if (!showOriginalStackTrace) copyStackTrace(error, stackTraceError) reject(error) return } + // we *could* (maybe should?) use `advanceTimersToNextTimer` but it's + // possible that could make this loop go on forever if someone is using + // third party code that's setting up recursive timers so rapidly that + // the user's timer's don't get a chance to resolve. So we'll advance + // by an interval instead. (We have a test for this case). + advanceTimersWrapper(() => { + jest.advanceTimersByTime(interval) + }) + + // It's really important that checkCallback is run *before* we flush + // in-flight promises. To be honest, I'm not sure why, and I can't quite + // think of a way to reproduce the problem in a test, but I spent + // an entire day banging my head against a wall on this. + checkCallback() + + // In this rare case, we *need* to wait for in-flight promises + // to resolve before continuing. We don't need to take advantage + // of parallelization so we're fine. + // https://stackoverflow.com/a/59243586/971592 + // eslint-disable-next-line no-await-in-loop + await advanceTimersWrapper(async () => { + await new Promise(r => { + setTimeout(r, 0) + jest.advanceTimersByTime(0) + }) + }) + } + } else { + try { + checkContainerType(container) + } catch (e) { + reject(e) + return } + intervalId = setInterval(checkRealTimersCallback, interval) + const {MutationObserver} = getWindowFromNode(container) + observer = new MutationObserver(checkRealTimersCallback) + observer.observe(container, mutationObserverOptions) + checkCallback() } function onDone(error, result) { finished = true - clearTimeout(overallTimeout) - clearInterval(intervalId) + clearTimeout(overallTimeoutTimer) - observer.disconnect() + if (!usingJestFakeTimers) { + clearInterval(intervalId) + observer.disconnect() + } if (error) { reject(error) @@ -117,27 +124,15 @@ function waitFor( } } - function ensureInvariantTimers() { - const isUsingJestFakeTimers = jestFakeTimersAreEnabled() - if (wasUsingJestFakeTimers !== isUsingJestFakeTimers) { + function checkRealTimersCallback() { + if (jestFakeTimersAreEnabled()) { const error = new Error( - `Changed from using ${ - wasUsingJestFakeTimers ? 'fake timers' : 'real timers' - } to ${ - isUsingJestFakeTimers ? 'fake timers' : 'real timers' - } while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing what timers your test is using. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, + `Changed from using real timers to fake timers while using waitFor. This is not allowed and will result in very strange behavior. Please ensure you're awaiting all async things your test is doing before changing to fake timers. For more info, please go to https://github.com/testing-library/dom-testing-library/issues/830`, ) if (!showOriginalStackTrace) copyStackTrace(error, stackTraceError) - throw error - } - } - - function handleInterval() { - try { - ensureInvariantTimers() - return checkCallback() - } catch (error) { return reject(error) + } else { + return checkCallback() } }