From a0bb0050b1ccb7652a60b72d617453400c300837 Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Tue, 3 Oct 2023 12:18:27 -0400 Subject: [PATCH 1/7] prevent waitFor callback racing condition --- src/wait-for.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/wait-for.js b/src/wait-for.js index ab2c9989..8e43186b 100644 --- a/src/wait-for.js +++ b/src/wait-for.js @@ -106,6 +106,9 @@ function waitFor( } function onDone(error, result) { + if (finished) { + return + } finished = true clearTimeout(overallTimeoutTimer) @@ -134,7 +137,7 @@ function waitFor( } function checkCallback() { - if (promiseStatus === 'pending') return + if (finished || promiseStatus === 'pending') return try { const result = runWithExpensiveErrorDiagnosticsDisabled(callback) if (typeof result?.then === 'function') { @@ -160,6 +163,9 @@ function waitFor( } function handleTimeout() { + if (finished) { + return + } let error if (lastError) { error = lastError From 6b41483386804687e145119bc1966a2da711eb01 Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Tue, 3 Oct 2023 15:18:27 -0400 Subject: [PATCH 2/7] change logic --- src/wait-for.js | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/wait-for.js b/src/wait-for.js index 8e43186b..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 { @@ -106,9 +106,6 @@ function waitFor( } function onDone(error, result) { - if (finished) { - return - } finished = true clearTimeout(overallTimeoutTimer) @@ -137,7 +134,7 @@ function waitFor( } function checkCallback() { - if (finished || promiseStatus === 'pending') return + if (promiseStatus === 'pending') return try { const result = runWithExpensiveErrorDiagnosticsDisabled(callback) if (typeof result?.then === 'function') { @@ -163,9 +160,6 @@ function waitFor( } function handleTimeout() { - if (finished) { - return - } let error if (lastError) { error = lastError From 55ff2f25d46d40331a36643aebafc1cce63dbcca Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Wed, 4 Oct 2023 09:33:02 -0400 Subject: [PATCH 3/7] adapt the waitFor resolve test to make it more readable --- src/__tests__/wait-for.js | 44 +++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index d956525d..98d306ca 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,32 +306,31 @@ 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') } }, }) @@ -342,6 +340,7 @@ test('does not work after it resolves', async () => { data = 'resolved' }, 100) + contextStack.push('waitFor:start') await waitFor( () => { // eslint-disable-next-line jest/no-conditional-in-test -- false-positive @@ -351,10 +350,29 @@ test('does not work after it resolves', async () => { }, {interval: 50}, ) - - expect(context).toEqual('initial') + contextStack.push('waitFor:end') + + expect(contextStack).toEqual([ + 'waitFor:start', + 'no-act:start', + 'act:start', + 'act:end', + 'act:start', + 'act:end', + 'no-act:end', + 'waitFor:end', + ]) await Promise.resolve() - expect(context).toEqual('initial') + expect(contextStack).toEqual([ + 'waitFor:start', + 'no-act:start', + 'act:start', + 'act:end', + 'act:start', + 'act:end', + 'no-act:end', + 'waitFor:end', + ]) }) From 67793f890106406e36cb84669276d203dc11aa1c Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Wed, 4 Oct 2023 09:46:12 -0400 Subject: [PATCH 4/7] surface `timeout` + waitFor `callback` calls --- src/__tests__/wait-for.js | 54 ++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index 98d306ca..f8ee14b5 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -337,12 +337,14 @@ test('does not work after it resolves', async () => { let data = null setTimeout(() => { + contextStack.push('timeout') data = 'resolved' }, 100) contextStack.push('waitFor:start') await waitFor( () => { + contextStack.push('callback') // eslint-disable-next-line jest/no-conditional-in-test -- false-positive if (data === null) { throw new Error('not found') @@ -352,27 +354,39 @@ test('does not work after it resolves', async () => { ) contextStack.push('waitFor:end') - expect(contextStack).toEqual([ - 'waitFor:start', - 'no-act:start', - 'act:start', - 'act:end', - 'act:start', - 'act:end', - 'no-act:end', - '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(contextStack).toEqual([ - 'waitFor:start', - 'no-act:start', - 'act:start', - 'act:end', - 'act:start', - 'act:end', - 'no-act:end', - '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, + ] + `) }) From 86c5c0882e55b8e2f8c0935fd9df4c3770357641 Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Wed, 4 Oct 2023 09:47:28 -0400 Subject: [PATCH 5/7] adding comment --- src/__tests__/wait-for.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index f8ee14b5..6c570610 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -373,6 +373,7 @@ test('does not work after it resolves', async () => { await Promise.resolve() + // The context call stack should not change expect(contextStack).toMatchInlineSnapshot(` [ waitFor:start, From ce11c0f9f51f51f1e84cb790016d83f44502ec87 Mon Sep 17 00:00:00 2001 From: Kevin Bon Date: Wed, 4 Oct 2023 12:19:47 -0400 Subject: [PATCH 6/7] adding reproductible test --- src/__tests__/wait-for.js | 67 +++++++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index 6c570610..d0a217ba 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -335,10 +335,10 @@ test('does not work after it resolves', async () => { }, }) - let data = null + let timeoutResolved = false setTimeout(() => { contextStack.push('timeout') - data = 'resolved' + timeoutResolved = true }, 100) contextStack.push('waitFor:start') @@ -346,7 +346,7 @@ test('does not work after it resolves', async () => { () => { contextStack.push('callback') // eslint-disable-next-line jest/no-conditional-in-test -- false-positive - if (data === null) { + if (!timeoutResolved) { throw new Error('not found') } }, @@ -391,3 +391,64 @@ test('does not work after it resolves', async () => { ] `) }) + +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" */ + } + }, + }) + + let waitForHasResolved = false + let callbackCalledAfterWaitForResolved = false + + await expect(() => + waitFor( + () => { + // eslint-disable-next-line jest/no-conditional-in-test -- false-positive + if (waitForHasResolved) { + callbackCalledAfterWaitForResolved = true + } + throw new Error('We want to timeout') + }, + {interval: 50, timeout: 100}, + ), + ).rejects.toThrow() + waitForHasResolved = true + + await Promise.resolve() + + expect(callbackCalledAfterWaitForResolved).toBe(false) +}) From 10a1ac7c52e8c027959bf7ff1c7368b77bd56168 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Mon, 8 Jan 2024 21:08:05 +0100 Subject: [PATCH 7/7] robuster assertion for not calling callback again --- src/__tests__/wait-for.js | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/__tests__/wait-for.js b/src/__tests__/wait-for.js index d0a217ba..5295543c 100644 --- a/src/__tests__/wait-for.js +++ b/src/__tests__/wait-for.js @@ -431,24 +431,19 @@ test(`when fake timer is installed, on waitFor timeout, it doesn't call the call }, }) - let waitForHasResolved = false - let callbackCalledAfterWaitForResolved = false + const callback = jest.fn(() => { + throw new Error('We want to timeout') + }) + const interval = 50 await expect(() => - waitFor( - () => { - // eslint-disable-next-line jest/no-conditional-in-test -- false-positive - if (waitForHasResolved) { - callbackCalledAfterWaitForResolved = true - } - throw new Error('We want to timeout') - }, - {interval: 50, timeout: 100}, - ), + waitFor(callback, {interval, timeout: 100}), ).rejects.toThrow() - waitForHasResolved = true + expect(callback).toHaveBeenCalledWith() - await Promise.resolve() + callback.mockClear() + + await jest.advanceTimersByTimeAsync(interval) - expect(callbackCalledAfterWaitForResolved).toBe(false) + expect(callback).not.toHaveBeenCalledWith() })