[Bug]: jest.restoreAllMocks() clobbers legacy fake timers since 29.4.3

[robhogan]

Version

29.4.3

Steps to reproduce

1. git clone https://github.com/robhogan/jest-restore-mocks-bug
2. Observe npm --prefix jest-29-4-2 test passes
3. npm --prefix jest-29-4-3 test fails

Expected behavior

Tests that pass in 29.4.2 should pass in 29.4.3

Actual behavior

Test fails in 29.4.3

Additional context

I imagine this has a lot to do with the various changes to restoreAllMocks coupled with the fact that legacy timers are Jest mock based. Calling useFakeTimers({ legacyFakeTimers: true }) after jest.restoreAllMocks() fixes the test again.

I'm still looking into root-causing and fixing this but thought I'd share a repro as soon as I had one.

Environment

System:
    OS: macOS 13.4.1
    CPU: (10) arm64 Apple M1 Max
  Binaries:
    Node: 20.1.0 - ~/.nvm/versions/node/v20.1.0/bin/node
    Yarn: 1.22.19 - ~/homebrew/bin/yarn
    npm: 9.6.4 - ~/.nvm/versions/node/v20.1.0/bin/npm
    pnpm: 7.27.1 - ~/homebrew/bin/pnpm
  npmPackages:
    jest: ~29.4.0 => 29.4.3

[SimenB]

@mrazauskas any immediate ideas?

[mrazauskas]

First though to check if time is faked (and that legacy implementation was used) and if so to call useFakeTimers({ legacyFakeTimers: true }) here:

jest/packages/jest-runtime/src/index.ts

Lines 1334 to 1336 in 0fd5b1c

	restoreAllMocks(): void {
	this._moduleMocker.restoreAllMocks();
	}

Fake timers will get installed, but is that enough? These will be not the same instances of mock functions. Second thought from here: jest.resetAllMocks() and jest.clearAllMocks() also mess up timer mock functions. Perhaps it would be enough to document all these peculiarities of legacy timers? In a way the following makes sense (or not?):

beforeEach(() => {
  jest.restoreAllMocks();
  jest.useFakeTimers({ legacyFakeTimers: true });
});

---

By the way, jest.restoreAllMocks() is in the spotlight, but what happens after calling jest.resetAllMocks()? If in the reproduction I use jest.resetAllMocks() instead of jest.restoreAllMocks(), test fails with both versions of Jest. How do users deal with that? Sorry, I never used legacy timers (;

[robhogan]

Thanks for the prompt response folks!

jest.resetAllMocks() and jest.clearAllMocks() also mess up timer mock functions.

This is a fair point, but as you say, in those cases the "test fails with both versions of Jest". The "bug" here isn't so much that 29.4.3 behaves in a certain way, it's that it behaves differently from 29.4.2. In our case that's breaking a few dozen test suites and blocks upgrading within v29.

Interestingly, it looks like

jest/e2e/fake-timers-legacy/reset-all-mocks/__tests__/resetAllMocks.test.js

Lines 10 to 16 in 0fd5b1c

	test('works before resetAllMocks is called', () => {
	jest.useFakeTimers();
	const f = jest.fn();
	setTimeout(f, 0);
	jest.runAllTimers();
	expect(f).toHaveBeenCalledTimes(1);
	});

...was intended to prevent resetAllMocks() from interfering with useFakeTimers(), but the test seems to omit actually calling resetAllMocks(). Maybe resetAllMocks() used to play nicely with useFakeTimers, but stopped working in a previous version?

Explicitly reinstalling timers after restoring mocks would work I think, but it's messy when timers are otherwise configured at the global/project level, and would make the migration to modern timers more difficult. I'd rather test authors not have to be worrying about the implementation details of timers.

I know 29.4.3 is a few versions ago, but IMO a patch restoring previous behaviour and holding breaking changes until v30 would be worth considering here.

Sorry, I never used legacy timers

I wish we could get to that point 😅. We'd love to. Unfortunately, that's a pretty huge migration effort I was hoping not to have to think about for a minor bump.

[mrazauskas]

Thanks for pointing to this test suite. Note that it has a second test with these lines (legacy timers are used, because of "legacyFakeTimers": true in package.json):

jest/e2e/fake-timers-legacy/reset-all-mocks/__tests__/resetAllMocks.test.js

Lines 19 to 20 in 0fd5b1c

	jest.resetAllMocks();
	jest.useFakeTimers();

File history is somewhat broken on GitHub. The test file was added in 2016 and got just few cosmetic changes (see #2109). This answers my question how jest.resetAllMocks() is supposed to be used with legacy timers. So I guess jest.restoreAllMocks() should be used in similar way.

---

The "bug" here isn't so much that 29.4.3 behaves in a certain way, it's that it behaves differently from 29.4.2.

Hm.. Isn’t it that we could complain about any bug fix in similar way? The change we are talking about comes from #13867. Looking at the PR, you can see that this was just a tiny bug fix.

I understand that the change is painful and that it is rather unfortunate to publish it as a minor release. All I could do is to spend time trying to explain how Jest got better after this change (see #13925). Hope this helps.

[robhogan]

Thanks for linking that context. It may well be that new behaviour is better designed, more consistent, etc - I’m not disagreeing with the change in principle - but as it was an apparently unintentional regression (breaks tests that previously passed), shouldn’t we follow semver, revert it and hold it back until v30?

To put it directly, would you accept a PR that reverted the behaviour change, or is this effectively “wontfix”?

[SimenB]

I'm happy to land a revert and then revert the revert for v30

[mrazauskas]

@robhogan This can be closed, right?

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.