Revert breaking changes to mocks (restoreAllMocks, spy changes) since 29.3.0 #14429
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
docs/MockFunctionAPI.md
Outdated
| @@ -130,7 +130,7 @@ Beware that `mockFn.mockClear()` will replace `mockFn.mock`, not just reset the | |||
|
|
|||
| Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also removes any mocked return values or implementations. | |||
|
|
|||
| This is useful when you want to completely reset a _mock_ back to its initial state. | |||
| This is useful when you want to completely reset a _mock_ back to its initial state. (Note that resetting a _spy_ will result in a function with no return value). | |||
There was a problem hiding this comment.
should it be re-added to older versions of the docs as well?
There was a problem hiding this comment.
Maybe. I'd reconsider the wording though, I think. The behaviour (pre 29.4.0 / after this PR) is to replace all spy mock implementations with empty functions - that is, it's not just about the return value.
test('mocks, spies, resetAllMocks', () => {
jest.spyOn(console, 'log');
jest.spyOn(globalThis, 'setImmediate').mockImplementation(cb => cb());
const mockCb = jest.fn();
setImmediate(mockCb); // Calls mockCb synchronously
expect(mockCb).toHaveBeenCalled();
// spyOn wraps original implementation, foo printed to console
console.log('foo');
expect(console.log).toHaveBeenCalledWith('foo');
jest.resetAllMocks();
// Replaces default spy implementation with a stub - nothing is printed.
console.log('bar');
expect(console.log.mock.calls).toEqual([['bar']]);
setImmediate(mockCb); // Never calls mockCb
expect(mockCb).not.toHaveBeenCalled();
});This is in fact what the paragraph above the one in question already says it should do, and IMO the confusion only arises from "Back to its initial state", which leads to people expecting it to do what mockRestore does (and possibly led to #13808).
Instead of:
Does everything that
mockFn.mockClear()does, and also removes any mocked return values or implementations.This is useful when you want to completely reset a mock back to its initial state.
How about:
Does everything that
mockFn.mockClear()does, and also replaces the mock implementation with an empty function, returningundefined.
There was a problem hiding this comment.
yeah good point. I think your suggestion sounds good 👍
There was a problem hiding this comment.
Updated and rebased 👍
…()` is called (jestjs#13866)" This reverts commit 9432fc3.
…alled (jestjs#13867)" This reverts commit 66fb417.
…jestjs#13692)" This reverts commit eace3a1.
robhogan
force-pushed
the
robhogan/revert-mock-changes
branch
from
a41537b
to
abced97
Compare
|
This PR is somewhat puzzling for me.
Those aren’t just "our tests are not passing" problems. These are in the level "Jest does not run for us anymore". So would a revert PRs be considered as well? |
|
Yeah, I'd consider those revert candidates. Semver is pretty clear on this:
Maybe it'd be worth considering moving development of a major onto branches as soon as a major is cut (eg, any commits that are
|
|
Thanks for bringing in those lines from Semver spec here. They answered all questions I had. Somehow I was sure that any bug fix can be released as a patch, but the spec talks about "backward compatible bug fixes" only. That sounds smarter than I was thinking. |
|
I don't really think the steam locomotive case should warrant a revert (even though it's weird Sapling itself doesn't provide an alternative binary) - it's people trolling themselves. But the symbol thing sounds like it probably should be reverted. IMO, any "backward incompatible changes" is too broad, but it's very good general advice. But if you rely on a bug, and change to its behaviour shouldn't necessarily lead to a new major. Extreme example, but obligatory: https://xkcd.com/1172/ |
|
@mrazauskas would you be up for preparing a PR essentially reverting this one again so we remember to reland for v30? |
|
Thanks both! Re the 085f063#diff-7ae0bd704c3c2789b19abe2bbf94aca3505e2a6b3823d85c7f6b316b216d37c9L1461-R1419 The change reverted above, and in particular |
Documentation starts with "Restores all mocks" and "Equivalent to calling Just above in the jest.spyOn(object, methodName).mockImplementation(() => customImplementation)
object[methodName] = jest.fn(() => customImplementation)For me it sounds like the second case is that "other mock" which has to be manually restored. The "other mocks" are "all mock" and |
|
That's fair - I think we can agree that the docs on this are not unambiguous. The current behaviour does match "no effect on non-spies" though, and has done for a long time. If that's to change, it might be worth considering giving folks an easy migration path. FWIW, this broke almost every test using My interpretation of |
Broke because of jestjs/jest#14429
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
A series of commits to
jest-mockthat landed in Jest 29.4.0-29.4.3 introduced breaking behaviour changes. Per semver, and especially because Jest's multi-package architecture make it difficult to "pin" to an old version, these kind of changes should be held until at least Jest 30.Changes reverted:
jest.resetAllMocks()is called #13866jest.restoreAllMocks()is called #13867In practice, we found this broke a few things, but the biggest by far was the change to
restoreAllMockssuch that it became equivalent to callingmockRestoreon every mocked function. Note that this contradicts the docs:Mocks created "manually" by assigning
jest.fn(), egconst myMock = jest.fn().mockReturnValue('foo')orglobal.setImmediate = jest.fn(cb => cb())lost their implementation whenrestoreAllMockswas called. (Note that they're not really "restored" to any state they had previously, and they're still mocks). In particular this breaks legacy fake timers, and mocks created during setup to emulate an environment (popular forrequestAnimationFrame, etc).(Suggestion: If we do bring these changes back for Jest 30, it'd be great to have a
restoreAllSpies()that does what the currentrestoreAllMocks()does - that'd make the migration much easier)Related / fixes
jest.restoreAllMocks()clobbers legacy fake timers since 29.4.3 #14390#13229 should be reopened if this is merged
Test plan
jest-mockin an otherwise Jest 29.6.2 installation) against ~28k tests at FB, and everything passes that passed in 29.2.1