runWithRealTimers & jestFakeTimersAreEnabled invalidate all pending (modern) fake timers

[9still]

• @testing-library/dom version: 7.29.5 (first seen), still present in 7.31.0
• jest version: 26.6.3
• jsdom version: 16.4.0

Relevant code or config:

You can reproduce the issue by adding the following code into the test for helpers.js:

  describe('runWithRealTimers/jestFakeTimersAreEnabled should be non-intrusive', () => {
    test('runAllTimers works without jestFakeTimersAreEnabled', async () => {
      jest.useFakeTimers('modern');

      const myPromise = new Promise((resolve) => {
        setTimeout(() => {
          resolve('done');
        }, 300);
      });

      jest.runAllTimers();

      await myPromise;
    });

    test('runAllTimers fails with jestFakeTimersAreEnabled', async () => {
      jest.useFakeTimers('modern');

      const myPromise = new Promise((resolve) => {
        setTimeout(() => {
          resolve('done');
        }, 300);
      });

      jestFakeTimersAreEnabled();
      jest.runAllTimers();

      await myPromise;
    });

    test('runAllTimers fails with runWithRealTimers', async () => {
      jest.useFakeTimers('modern');

      const myPromise = new Promise((resolve) => {
        setTimeout(() => {
          resolve('done');
        }, 300);
      });

      runWithRealTimers(() => {});

      jest.runAllTimers();

      await myPromise;
    });
  });

What you did:

Ran the test above

What happened:

Tests using the affected functions failed

    runWithRealTimers/jestFakeTimersAreEnabled should be non-intrusive
      ✓ runAllTimers works without jestFakeTimersAreEnabled
      ✕ runAllTimers fails with jestFakeTimersAreEnabled (5006 ms)
      ✕ runAllTimers fails with runWithRealTimers (5002 ms)

Reproduction:

Repro repo: https://github.com/9still/testing-library-dom-fake-timers-bug
Reproduce with npm t

Problem description:

runWithRealTimers & jestFakeTimersAreEnabled are basically broken when used with modern jest timers. Once jest 27 is out & modern timers become the default, this is likely going to quickly become a problem for anyone that upgrades.

We started noticing the issue starting with version 7.29.5 due to the change introduced in #887.

What is happening is that both jestFakeTimersAreEnabled and runWithRealTimers toggle jest from modern fake timers (initial state) to real timers & back to modern fake timers. Unfortunately this ends up creating a brand new sinon clock instance that is basically oblivious to the existence of any timers that were created prior to the reinitialization. Because of this, jest.runAllTimers & friends are unaware of any previously created timers & any tests relying on them basically time out.

Starting with #887 jestFakeTimersAreEnabled toggles jest timers from fake to real to fake again & given how frequently this method is called by various testing library methods, a test case in our app was toggling timers > 30000 times, where previously no toggling was done.

We effectively can't use modern timers in our codebase any more (note that legacy timers appear to work fine).

Suggested solution:

It seems that for jestFakeTimersAreEnabled you would get pretty far by doing something very simple & much less intrusive:

const jestFakeTimersAreEnabled = () => (!!setTimeout.clock || !!setTimeout._isMockFunction);

runWithRealTimers presents a much bigger problem however. You could try to do something clever and invasive & try to save the clock instance when modern timers are detected & manually restore it, but that'll probably be rather fragile.

What about going a whole different direction & instead of trying to figure out what jest was using, you simply required that the testing lib be initialized with the real timer functions & saved references to them at module scope (prior to the user ever invoking useFakeTimers or otherwise stubbing things in their test suite?

Of course once you had the originals, you'd need to manually replace the affected methods as needed & restore them back to the originals while leaving the jest timers completely alone & without having to worry about interfering with the test suite - it'd be reimplementing jest functionality, but it feels like there isn't much choice here...

[timdeschryver]

Hi, I think we can close this one in favor of #939.
If I'm wrong, feel free to re-open.