From 6b215e622829ff79a307f831a62e47817543772a Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Wed, 3 Feb 2021 08:44:29 +0100 Subject: [PATCH 1/7] wip: analyze leaking test --- src/__tests__/helpers.js | 81 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/src/__tests__/helpers.js b/src/__tests__/helpers.js index 1bc85837..24242e43 100644 --- a/src/__tests__/helpers.js +++ b/src/__tests__/helpers.js @@ -53,6 +53,87 @@ describe('query container validation throws when validation fails', () => { }) }) +describe('analyze test leak', () => { + const realSetTimeout = global.setTimeout + + afterEach(() => { + global.setTimeout = realSetTimeout + }) + + const testReal = () => { + const originalSetTimeout = globalObj.setTimeout + + // legacy timers use mocks and do not rely on a clock instance + jest.useFakeTimers('legacy') + runWithRealTimers(() => { + expect(originalSetTimeout).toEqual(globalObj.setTimeout) + }) + expect(globalObj.setTimeout._isMockFunction).toBe(true) + expect(globalObj.setTimeout.clock).toBeUndefined() + + jest.useRealTimers() + + // modern timers use a clock instance instead of a mock + jest.useFakeTimers('modern') + runWithRealTimers(() => { + expect(originalSetTimeout).toEqual(globalObj.setTimeout) + }) + expect(globalObj.setTimeout._isMockFunction).toBeUndefined() + expect(globalObj.setTimeout.clock).toBeDefined() + } + const testFake = () => { + const originalSetTimeout = globalObj.setTimeout + + // useFakeTimers is not used, timers are faked in some other way + const fakedSetTimeout = callback => { + callback() + } + fakedSetTimeout.clock = jest.fn() + + globalObj.setTimeout = fakedSetTimeout + + runWithRealTimers(() => { + expect(fakedSetTimeout).toEqual(globalObj.setTimeout) + }) + + globalObj.setTimeout = originalSetTimeout + } + + test('jest.useRealTimers is safe to call', () => { + expect(global.setTimeout).toBe(realSetTimeout) + + jest.useRealTimers() + expect(global.setTimeout).toBe(realSetTimeout) + }) + + test('jest.useRealTimers is safe to call after test code', () => { + expect(global.setTimeout).toBe(realSetTimeout) + + testFake() + expect(global.setTimeout).toBe(realSetTimeout) + + jest.useRealTimers() + expect(global.setTimeout).toBe(realSetTimeout) + }) + + test('jest.useRealTimers is not safe to call after both test codes', () => { + expect(global.setTimeout).toBe(realSetTimeout) + + testReal() + expect(global.setTimeout).not.toBe(realSetTimeout) + + jest.useRealTimers() + expect(global.setTimeout).toBe(realSetTimeout) + + testFake() + expect(global.setTimeout).toBe(realSetTimeout) + + jest.useRealTimers() + // THIS SHOULD BE REAL - pre-commit-hook doesn't allow to commit failing tests :( + expect(global.setTimeout).not.toBe(realSetTimeout) + }) +}) + test('should always use realTimers before using callback when timers are faked with useFakeTimers', () => { const originalSetTimeout = globalObj.setTimeout From 16a2265ee07bd1c3e7b5be3813b8ca52e7d266a6 Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Wed, 3 Feb 2021 09:31:12 +0100 Subject: [PATCH 2/7] wip: analyze leaking test --- src/__tests__/helpers.js | 46 ++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/__tests__/helpers.js b/src/__tests__/helpers.js index 24242e43..6eb9c92e 100644 --- a/src/__tests__/helpers.js +++ b/src/__tests__/helpers.js @@ -60,27 +60,6 @@ describe('analyze test leak', () => { global.setTimeout = realSetTimeout }) - const testReal = () => { - const originalSetTimeout = globalObj.setTimeout - - // legacy timers use mocks and do not rely on a clock instance - jest.useFakeTimers('legacy') - runWithRealTimers(() => { - expect(originalSetTimeout).toEqual(globalObj.setTimeout) - }) - expect(globalObj.setTimeout._isMockFunction).toBe(true) - expect(globalObj.setTimeout.clock).toBeUndefined() - - jest.useRealTimers() - - // modern timers use a clock instance instead of a mock - jest.useFakeTimers('modern') - runWithRealTimers(() => { - expect(originalSetTimeout).toEqual(globalObj.setTimeout) - }) - expect(globalObj.setTimeout._isMockFunction).toBeUndefined() - expect(globalObj.setTimeout.clock).toBeDefined() - } const testFake = () => { const originalSetTimeout = globalObj.setTimeout @@ -99,14 +78,27 @@ describe('analyze test leak', () => { globalObj.setTimeout = originalSetTimeout } - test('jest.useRealTimers is safe to call', () => { + test('jest.useRealTimers is safe to call after testFake code', () => { + expect(global.setTimeout).toBe(realSetTimeout) + + testFake() expect(global.setTimeout).toBe(realSetTimeout) jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) }) - test('jest.useRealTimers is safe to call after test code', () => { + test('jest.useRealTimers is safe to call after legacy fake timer and testFake code', () => { + expect(global.setTimeout).toBe(realSetTimeout) + + jest.useFakeTimers('legacy') + expect(global.setTimeout).not.toBe(realSetTimeout) + + runWithRealTimers(() => { + expect(global.setTimeout).toBe(realSetTimeout) + }) + + jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) testFake() @@ -116,12 +108,16 @@ describe('analyze test leak', () => { expect(global.setTimeout).toBe(realSetTimeout) }) - test('jest.useRealTimers is not safe to call after both test codes', () => { + test('FAIL: jest.useRealTimers is safe to call after modern fake timers and testFake code', () => { expect(global.setTimeout).toBe(realSetTimeout) - testReal() + jest.useFakeTimers('modern') expect(global.setTimeout).not.toBe(realSetTimeout) + runWithRealTimers(() => { + expect(global.setTimeout).toBe(realSetTimeout) + }) + jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) From 24ebf282d10782b3d0a22b939a1b2950d4b5bd1b Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Wed, 3 Feb 2021 11:18:06 +0100 Subject: [PATCH 3/7] wip: demonstrate bug in runWithRealTimers --- src/__tests__/helpers.js | 72 +++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 42 deletions(-) diff --git a/src/__tests__/helpers.js b/src/__tests__/helpers.js index 6eb9c92e..7fd9d459 100644 --- a/src/__tests__/helpers.js +++ b/src/__tests__/helpers.js @@ -60,72 +60,60 @@ describe('analyze test leak', () => { global.setTimeout = realSetTimeout }) - const testFake = () => { - const originalSetTimeout = globalObj.setTimeout - - // useFakeTimers is not used, timers are faked in some other way - const fakedSetTimeout = callback => { - callback() - } - fakedSetTimeout.clock = jest.fn() - - globalObj.setTimeout = fakedSetTimeout - - runWithRealTimers(() => { - expect(fakedSetTimeout).toEqual(globalObj.setTimeout) - }) - - globalObj.setTimeout = originalSetTimeout - } - - test('jest.useRealTimers is safe to call after testFake code', () => { + test('jest.getRealSystemTime does not throw after modern timers have been used', () => { expect(global.setTimeout).toBe(realSetTimeout) - testFake() - expect(global.setTimeout).toBe(realSetTimeout) + expect(jest.getRealSystemTime).toThrow() jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) - }) - - test('jest.useRealTimers is safe to call after legacy fake timer and testFake code', () => { - expect(global.setTimeout).toBe(realSetTimeout) - - jest.useFakeTimers('legacy') - expect(global.setTimeout).not.toBe(realSetTimeout) - runWithRealTimers(() => { - expect(global.setTimeout).toBe(realSetTimeout) - }) + jest.useFakeTimers('modern') + expect(jest.getRealSystemTime).not.toThrow() jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) - testFake() - expect(global.setTimeout).toBe(realSetTimeout) + // current implementation assumes this throws + // see https://github.com/testing-library/dom-testing-library/blob/5bc93643f312d9ca4210b97681686c9aa8a902d7/src/helpers.js#L43-L45 + expect(jest.getRealSystemTime).not.toThrow() + // jest.useRealTimers() is still no problem jest.useRealTimers() expect(global.setTimeout).toBe(realSetTimeout) }) - test('FAIL: jest.useRealTimers is safe to call after modern fake timers and testFake code', () => { + test('BUG: runWithRealTimers fakes timers after modern fake timers have been used', () => { + jest.useFakeTimers('modern') + jest.useRealTimers() + + // modern fake timers have been used, but the current timeout is the real one expect(global.setTimeout).toBe(realSetTimeout) - jest.useFakeTimers('modern') - expect(global.setTimeout).not.toBe(realSetTimeout) + // repeat the test for the fake implementation + const fakedSetTimeout = callback => { + callback() + } + fakedSetTimeout.clock = jest.fn() + global.setTimeout = fakedSetTimeout + // runWithRealTimers assumes jest.getRealSystemTime() would throw, but it does not + // it calls jest.useRealTimers() before the callback - which does nothing because no fake is active + // it calls jest.useFakeTimers('modern') after the callback runWithRealTimers(() => { - expect(global.setTimeout).toBe(realSetTimeout) + expect(fakedSetTimeout).toEqual(globalObj.setTimeout) }) - jest.useRealTimers() - expect(global.setTimeout).toBe(realSetTimeout) + // it should be fakedSetTimeout + expect(global.setTimeout).not.toBe(fakedSetTimeout) - testFake() - expect(global.setTimeout).toBe(realSetTimeout) + // this line makes the bug hard to track down + // without it jest.useRealTimers() will restore fakedSetTimeout + // with it jest.useRealTimers() 'restores' setTimeout to undefined + global.setTimeout = realSetTimeout + // now useRealTimers is broken jest.useRealTimers() - // THIS SHOULD BE REAL - pre-commit-hook doesn't allow to commit failing tests :( expect(global.setTimeout).not.toBe(realSetTimeout) }) }) From c7874917527a619adad82f482093020d7e994738 Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Wed, 3 Feb 2021 12:23:30 +0100 Subject: [PATCH 4/7] fix: remove side-effect from runWithRealTimers #884 #886 --- src/__tests__/helpers.js | 109 ++++++++------------------------------- src/helpers.js | 60 +++++++++------------ 2 files changed, 46 insertions(+), 123 deletions(-) diff --git a/src/__tests__/helpers.js b/src/__tests__/helpers.js index 7fd9d459..85d26ac9 100644 --- a/src/__tests__/helpers.js +++ b/src/__tests__/helpers.js @@ -5,10 +5,6 @@ import { runWithRealTimers, } from '../helpers' -const globalObj = typeof window === 'undefined' ? global : window - -afterEach(() => jest.useRealTimers()) - test('returns global document if exists', () => { expect(getDocument()).toBe(document) }) @@ -53,107 +49,44 @@ describe('query container validation throws when validation fails', () => { }) }) -describe('analyze test leak', () => { +describe('run with real timers', () => { const realSetTimeout = global.setTimeout afterEach(() => { + jest.useRealTimers() global.setTimeout = realSetTimeout }) - test('jest.getRealSystemTime does not throw after modern timers have been used', () => { - expect(global.setTimeout).toBe(realSetTimeout) - - expect(jest.getRealSystemTime).toThrow() - - jest.useRealTimers() - expect(global.setTimeout).toBe(realSetTimeout) - - jest.useFakeTimers('modern') - expect(jest.getRealSystemTime).not.toThrow() - - jest.useRealTimers() - expect(global.setTimeout).toBe(realSetTimeout) - - // current implementation assumes this throws - // see https://github.com/testing-library/dom-testing-library/blob/5bc93643f312d9ca4210b97681686c9aa8a902d7/src/helpers.js#L43-L45 - expect(jest.getRealSystemTime).not.toThrow() - - // jest.useRealTimers() is still no problem - jest.useRealTimers() - expect(global.setTimeout).toBe(realSetTimeout) + test('use real timers when timers are faked with jest.useFakeTimers(legacy)', () => { + // legacy timers use mocks and do not rely on a clock instance + jest.useFakeTimers('legacy') + runWithRealTimers(() => { + expect(global.setTimeout).toEqual(realSetTimeout) + }) + expect(global.setTimeout._isMockFunction).toBe(true) + expect(global.setTimeout.clock).toBeUndefined() }) - test('BUG: runWithRealTimers fakes timers after modern fake timers have been used', () => { + test('use real timers when timers are faked with jest.useFakeTimers(modern)', () => { + // modern timers use a clock instance instead of a mock jest.useFakeTimers('modern') - jest.useRealTimers() - - // modern fake timers have been used, but the current timeout is the real one - expect(global.setTimeout).toBe(realSetTimeout) + runWithRealTimers(() => { + expect(global.setTimeout).toEqual(realSetTimeout) + }) + expect(global.setTimeout._isMockFunction).toBeUndefined() + expect(global.setTimeout.clock).toBeDefined() + }) - // repeat the test for the fake implementation + test('do not use real timers when timers are not faked with jest.useFakeTimers', () => { + // useFakeTimers is not used, timers are faked in some other way const fakedSetTimeout = callback => { callback() } fakedSetTimeout.clock = jest.fn() global.setTimeout = fakedSetTimeout - // runWithRealTimers assumes jest.getRealSystemTime() would throw, but it does not - // it calls jest.useRealTimers() before the callback - which does nothing because no fake is active - // it calls jest.useFakeTimers('modern') after the callback runWithRealTimers(() => { - expect(fakedSetTimeout).toEqual(globalObj.setTimeout) + expect(global.setTimeout).toEqual(fakedSetTimeout) }) - - // it should be fakedSetTimeout - expect(global.setTimeout).not.toBe(fakedSetTimeout) - - // this line makes the bug hard to track down - // without it jest.useRealTimers() will restore fakedSetTimeout - // with it jest.useRealTimers() 'restores' setTimeout to undefined - global.setTimeout = realSetTimeout - - // now useRealTimers is broken - jest.useRealTimers() - expect(global.setTimeout).not.toBe(realSetTimeout) }) }) - -test('should always use realTimers before using callback when timers are faked with useFakeTimers', () => { - const originalSetTimeout = globalObj.setTimeout - - // legacy timers use mocks and do not rely on a clock instance - jest.useFakeTimers('legacy') - runWithRealTimers(() => { - expect(originalSetTimeout).toEqual(globalObj.setTimeout) - }) - expect(globalObj.setTimeout._isMockFunction).toBe(true) - expect(globalObj.setTimeout.clock).toBeUndefined() - - jest.useRealTimers() - - // modern timers use a clock instance instead of a mock - jest.useFakeTimers('modern') - runWithRealTimers(() => { - expect(originalSetTimeout).toEqual(globalObj.setTimeout) - }) - expect(globalObj.setTimeout._isMockFunction).toBeUndefined() - expect(globalObj.setTimeout.clock).toBeDefined() -}) - -test('should not use realTimers when timers are not faked with useFakeTimers', () => { - const originalSetTimeout = globalObj.setTimeout - - // useFakeTimers is not used, timers are faked in some other way - const fakedSetTimeout = callback => { - callback() - } - fakedSetTimeout.clock = jest.fn() - - globalObj.setTimeout = fakedSetTimeout - - runWithRealTimers(() => { - expect(fakedSetTimeout).toEqual(globalObj.setTimeout) - }) - - globalObj.setTimeout = originalSetTimeout -}) diff --git a/src/helpers.js b/src/helpers.js index f686d46b..a37db61e 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -5,52 +5,42 @@ const TEXT_NODE = 3 // Currently this fn only supports jest timers, but it could support other test runners in the future. function runWithRealTimers(callback) { - const fakeTimersType = getJestFakeTimersType() - if (fakeTimersType) { + return _runWithRealTimers(callback).callbackReturnValue +} + +function _runWithRealTimers(callback) { + const timerAPI = { + clearImmediate, + clearInterval, + clearTimeout, + setImmediate, + setInterval, + setTimeout, + } + + // istanbul ignore else + if (jest) { jest.useRealTimers() } const callbackReturnValue = callback() - if (fakeTimersType) { - jest.useFakeTimers(fakeTimersType) - } + const usedJestFakeTimers = Object.keys(timerAPI).filter( + k => timerAPI[k] !== globalObj[k], + ).length - return callbackReturnValue -} - -function getJestFakeTimersType() { - // istanbul ignore if - if ( - typeof jest === 'undefined' || - typeof globalObj.setTimeout === 'undefined' - ) { - return null + if (usedJestFakeTimers) { + jest.useFakeTimers(timerAPI.setTimeout?.clock ? 'modern' : 'legacy') } - if ( - typeof globalObj.setTimeout._isMockFunction !== 'undefined' && - globalObj.setTimeout._isMockFunction - ) { - return 'legacy' - } - - if ( - typeof globalObj.setTimeout.clock !== 'undefined' && - typeof jest.getRealSystemTime !== 'undefined' - ) { - try { - // jest.getRealSystemTime is only supported for Jest's `modern` fake timers and otherwise throws - jest.getRealSystemTime() - return 'modern' - } catch { - // not using Jest's modern fake timers - } + return { + callbackReturnValue, + usedJestFakeTimers, } - return null } -const jestFakeTimersAreEnabled = () => Boolean(getJestFakeTimersType()) +const jestFakeTimersAreEnabled = () => + Boolean(_runWithRealTimers(() => {}).usedJestFakeTimers) // we only run our tests in node, and setImmediate is supported in node. // istanbul ignore next From f62a12a58c55d78d3c85d0cd58ec3cc7b8b206e1 Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Thu, 11 Feb 2021 11:10:48 +0100 Subject: [PATCH 5/7] test: add comments and assertions --- src/__tests__/helpers.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/__tests__/helpers.js b/src/__tests__/helpers.js index 85d26ac9..8e5d436f 100644 --- a/src/__tests__/helpers.js +++ b/src/__tests__/helpers.js @@ -53,7 +53,9 @@ describe('run with real timers', () => { const realSetTimeout = global.setTimeout afterEach(() => { + // restore timers replaced by jest.useFakeTimers() jest.useRealTimers() + // restore setTimeout replaced by assignment global.setTimeout = realSetTimeout }) @@ -61,7 +63,7 @@ describe('run with real timers', () => { // legacy timers use mocks and do not rely on a clock instance jest.useFakeTimers('legacy') runWithRealTimers(() => { - expect(global.setTimeout).toEqual(realSetTimeout) + expect(global.setTimeout).toBe(realSetTimeout) }) expect(global.setTimeout._isMockFunction).toBe(true) expect(global.setTimeout.clock).toBeUndefined() @@ -71,7 +73,7 @@ describe('run with real timers', () => { // modern timers use a clock instance instead of a mock jest.useFakeTimers('modern') runWithRealTimers(() => { - expect(global.setTimeout).toEqual(realSetTimeout) + expect(global.setTimeout).toBe(realSetTimeout) }) expect(global.setTimeout._isMockFunction).toBeUndefined() expect(global.setTimeout.clock).toBeDefined() @@ -86,7 +88,8 @@ describe('run with real timers', () => { global.setTimeout = fakedSetTimeout runWithRealTimers(() => { - expect(global.setTimeout).toEqual(fakedSetTimeout) + expect(global.setTimeout).toBe(fakedSetTimeout) }) + expect(global.setTimeout).toBe(fakedSetTimeout) }) }) From f74db309cafa22de770e2879e2e8e9a6b801ba28 Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Thu, 11 Feb 2021 11:40:33 +0100 Subject: [PATCH 6/7] fix: guard against ReferenceError in non-jest environment --- src/helpers.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/helpers.js b/src/helpers.js index a37db61e..d1ae9aa4 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -18,9 +18,10 @@ function _runWithRealTimers(callback) { setTimeout, } - // istanbul ignore else - if (jest) { + try { jest.useRealTimers() + } catch (e) { + // not running in jest environment } const callbackReturnValue = callback() From f6a6eda8b1eeef51e1b9b46f718b13e7ea4e7f3f Mon Sep 17 00:00:00 2001 From: Philipp Fritsche Date: Thu, 11 Feb 2021 12:17:30 +0100 Subject: [PATCH 7/7] refactor: adopt suggestions --- src/helpers.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/helpers.js b/src/helpers.js index d1ae9aa4..7b8d95e5 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -18,17 +18,16 @@ function _runWithRealTimers(callback) { setTimeout, } - try { + // istanbul ignore else + if (typeof jest !== 'undefined') { jest.useRealTimers() - } catch (e) { - // not running in jest environment } const callbackReturnValue = callback() - const usedJestFakeTimers = Object.keys(timerAPI).filter( - k => timerAPI[k] !== globalObj[k], - ).length + const usedJestFakeTimers = Object.entries(timerAPI).some( + ([name, func]) => func !== globalObj[name], + ) if (usedJestFakeTimers) { jest.useFakeTimers(timerAPI.setTimeout?.clock ? 'modern' : 'legacy')