From 9aaf71524beaeafdf9d8b690a4a78867fa50a5d2 Mon Sep 17 00:00:00 2001 From: Kevin BON Date: Mon, 8 Jan 2024 15:12:20 -0500 Subject: [PATCH 1/2] fix: Stop calling `waitFor` callback after timeout (#1271) * prevent waitFor callback racing condition * change logic * adapt the waitFor resolve test to make it more readable * surface `timeout` + waitFor `callback` calls * adding comment * adding reproductible test * robuster assertion for not calling callback again --------- Co-authored-by: Kevin Bon Co-authored-by: Sebastian Silbermann --- src/__tests__/wait-for.js | 121 +++++++++++++++++++++++++++++++++----- src/wait-for.js | 8 +-- 2 files changed, 109 insertions(+), 20 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index d956525d..5295543c 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -292,12 +292,11 @@ test('the real timers => fake timers error shows the original stack trace when c test('does not work after it resolves', async () => { jest.useFakeTimers('modern') - let context = 'initial' + const contextStack = [] configure({ // @testing-library/react usage to ensure `IS_REACT_ACT_ENVIRONMENT` is set when acting. unstable_advanceTimersWrapper: callback => { - const originalContext = context - context = 'act' + contextStack.push('act:start') try { const result = callback() // eslint-disable-next-line jest/no-if, jest/no-conditional-in-test -- false-positive @@ -307,54 +306,144 @@ test('does not work after it resolves', async () => { then: (resolve, reject) => { thenable.then( returnValue => { - context = originalContext + contextStack.push('act:end') resolve(returnValue) }, error => { - context = originalContext + contextStack.push('act:end') reject(error) }, ) }, } } else { - context = originalContext + contextStack.push('act:end') return undefined } } catch { - context = originalContext + contextStack.push('act:end') return undefined } }, asyncWrapper: async callback => { - const originalContext = context - context = 'no-act' + contextStack.push('no-act:start') try { await callback() } finally { - context = originalContext + contextStack.push('no-act:end') } }, }) - let data = null + let timeoutResolved = false setTimeout(() => { - data = 'resolved' + contextStack.push('timeout') + timeoutResolved = true }, 100) + contextStack.push('waitFor:start') await waitFor( () => { + contextStack.push('callback') // eslint-disable-next-line jest/no-conditional-in-test -- false-positive - if (data === null) { + if (!timeoutResolved) { throw new Error('not found') } }, {interval: 50}, ) - - expect(context).toEqual('initial') + contextStack.push('waitFor:end') + + expect(contextStack).toMatchInlineSnapshot(` + [ + waitFor:start, + no-act:start, + callback, + act:start, + act:end, + callback, + act:start, + timeout, + act:end, + callback, + no-act:end, + waitFor:end, + ] + `) await Promise.resolve() - expect(context).toEqual('initial') + // The context call stack should not change + expect(contextStack).toMatchInlineSnapshot(` + [ + waitFor:start, + no-act:start, + callback, + act:start, + act:end, + callback, + act:start, + timeout, + act:end, + callback, + no-act:end, + waitFor:end, + ] + `) +}) + +test(`when fake timer is installed, on waitFor timeout, it doesn't call the callback afterward`, async () => { + jest.useFakeTimers('modern') + + configure({ + // @testing-library/react usage to ensure `IS_REACT_ACT_ENVIRONMENT` is set when acting. + unstable_advanceTimersWrapper: callback => { + try { + const result = callback() + // eslint-disable-next-line jest/no-if, jest/no-conditional-in-test -- false-positive + if (typeof result?.then === 'function') { + const thenable = result + return { + then: (resolve, reject) => { + thenable.then( + returnValue => { + resolve(returnValue) + }, + error => { + reject(error) + }, + ) + }, + } + } else { + return undefined + } + } catch { + return undefined + } + }, + asyncWrapper: async callback => { + try { + await callback() + } finally { + /* eslint no-empty: "off" */ + } + }, + }) + + const callback = jest.fn(() => { + throw new Error('We want to timeout') + }) + const interval = 50 + + await expect(() => + waitFor(callback, {interval, timeout: 100}), + ).rejects.toThrow() + expect(callback).toHaveBeenCalledWith() + + callback.mockClear() + + await jest.advanceTimersByTimeAsync(interval) + + expect(callback).not.toHaveBeenCalledWith() }) diff --git a/src/wait-for.js b/src/wait-for.js index ab2c9989..4787d031 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -81,15 +81,15 @@ function waitFor( jest.advanceTimersByTime(interval) }) + // Could have timed-out + if (finished) { + break + } // 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() - - if (finished) { - break - } } } else { try { From bd04cf95a1ed85a2238f7dfc1a77d5d16b4f59dc Mon Sep 17 00:00:00 2001 From: "allcontributors[bot]" <46447321+allcontributors[bot]@users.noreply.github.com> Date: Mon, 8 Jan 2024 21:12:37 +0100 Subject: [PATCH 2/2] docs: add KevinBon as a contributor for code, and bug (#1284) * docs: update README.md * docs: update .all-contributorsrc --------- Co-authored-by: allcontributors[bot] <46447321+allcontributors[bot]@users.noreply.github.com> --- .all-contributorsrc | 10 ++++++++++ README.md | 1 + 2 files changed, 11 insertions(+) diff --git a/.all-contributorsrc b/.all-contributorsrc index e36b8dff..5b699465 100644 --- a/.all-contributorsrc +++ b/.all-contributorsrc @@ -1626,6 +1626,16 @@ "code", "bug" ] + }, + { + "login": "KevinBon", + "name": "Kevin BON", + "avatar_url": "https://avatars.githubusercontent.com/u/1910927?v=4", + "profile": "https://github.com/KevinBon", + "contributions": [ + "code", + "bug" + ] } ], "repoHost": "https://github.com", diff --git a/README.md b/README.md index 02a370c9..2323bb5b 100644 --- a/README.md +++ b/README.md @@ -346,6 +346,7 @@ Thanks goes to these people ([emoji key][emojis]): Craig Morten
Craig Morten
💻 Naor Peled
Naor Peled
💻 Julien Wajsberg
Julien Wajsberg
💻 🐛 + Kevin BON
Kevin BON
💻 🐛