From 085f06331668e8f041b29f3cfe6bb6513735b3f0 Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Mon, 21 Aug 2023 13:02:39 +0100 Subject: [PATCH] Revert breaking changes to mocks (restoreAllMocks, spy changes) since 29.3.0 (#14429) --- CHANGELOG.md | 3 + docs/MockFunctionAPI.md | 4 +- .../jest-mock/src/__tests__/index.test.ts | 163 +++++++++++------- packages/jest-mock/src/index.ts | 138 +++++---------- .../version-29.4/MockFunctionAPI.md | 4 +- .../version-29.5/MockFunctionAPI.md | 4 +- .../version-29.6/MockFunctionAPI.md | 4 +- 7 files changed, 151 insertions(+), 169 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bbc4accd8ca..010e2c3aa459 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - `[expect]` Remove `@types/node` from dependencies ([#14385](https://github.com/jestjs/jest/pull/14385)) - `[jest-core]` Use workers in watch mode by default to avoid crashes ([#14059](https://github.com/facebook/jest/pull/14059) & [#14085](https://github.com/facebook/jest/pull/14085)). - `[jest-reporters]` Update `istanbul-lib-instrument` dependency to v6. ([#14401](https://github.com/jestjs/jest/pull/14401)) +- `[jest-mock]` Revert [#13692](https://github.com/jestjs/jest/pull/13692) as it was a breaking change ([#14429](https://github.com/jestjs/jest/pull/14429)) +- `[jest-mock]` Revert [#13866](https://github.com/jestjs/jest/pull/13866) as it was a breaking change ([#14429](https://github.com/jestjs/jest/pull/14429)) +- `[jest-mock]` Revert [#13867](https://github.com/jestjs/jest/pull/13867) as it was a breaking change ([#14429](https://github.com/jestjs/jest/pull/14429)) ### Chore & Maintenance diff --git a/docs/MockFunctionAPI.md b/docs/MockFunctionAPI.md index 5ffb11992b84..6282e1538f2d 100644 --- a/docs/MockFunctionAPI.md +++ b/docs/MockFunctionAPI.md @@ -128,9 +128,7 @@ Beware that `mockFn.mockClear()` will replace `mockFn.mock`, not just reset the ### `mockFn.mockReset()` -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. +Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also replaces the mock implementation with an empty function, returning `undefined`. The [`resetMocks`](configuration#resetmocks-boolean) configuration option is available to reset mocks automatically before each test. diff --git a/packages/jest-mock/src/__tests__/index.test.ts b/packages/jest-mock/src/__tests__/index.test.ts index 7d08d17203df..c0efe908e7d9 100644 --- a/packages/jest-mock/src/__tests__/index.test.ts +++ b/packages/jest-mock/src/__tests__/index.test.ts @@ -598,6 +598,26 @@ describe('moduleMocker', () => { expect(fn2()).not.toBe('abcd'); }); + it('is not affected by restoreAllMocks', () => { + const fn1 = moduleMocker.fn(); + fn1.mockImplementation(() => 'abcd'); + fn1(1, 2, 3); + expect(fn1.mock.calls).toEqual([[1, 2, 3]]); + moduleMocker.restoreAllMocks(); + expect(fn1(1)).toBe('abcd'); + expect(fn1.mock.calls).toEqual([[1, 2, 3], [1]]); + }); + + it('is cleared and stubbed when restored explicitly', () => { + const fn1 = moduleMocker.fn(); + fn1.mockImplementation(() => 'abcd'); + fn1(1, 2, 3); + expect(fn1.mock.calls).toEqual([[1, 2, 3]]); + fn1.mockRestore(); + expect(fn1(1)).toBeUndefined(); + expect(fn1.mock.calls).toEqual([[1]]); + }); + it('maintains function arity', () => { const mockFunctionArity1 = moduleMocker.fn(x => x); const mockFunctionArity2 = moduleMocker.fn((x, y) => y); @@ -1547,18 +1567,21 @@ describe('moduleMocker', () => { }); it('supports resetting a spy', () => { - const methodOneReturn = 0; + const methodOneReturn = 10; + let methodOneRealCalls = 0; const obj = { methodOne() { + methodOneRealCalls++; return methodOneReturn; }, }; - const spy1 = moduleMocker.spyOn(obj, 'methodOne').mockReturnValue(10); + const spy1 = moduleMocker.spyOn(obj, 'methodOne').mockReturnValue(100); // Return value is mocked. - expect(methodOneReturn).toBe(0); - expect(obj.methodOne()).toBe(10); + expect(obj.methodOne()).toBe(100); + // Real impl has not been used. + expect(methodOneRealCalls).toBe(0); expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(true); @@ -1567,32 +1590,45 @@ describe('moduleMocker', () => { // After resetting the spy, the method is still mock functions. expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(true); - // After resetting the spy, the method returns the original return value. - expect(methodOneReturn).toBe(0); - expect(obj.methodOne()).toBe(0); + // After resetting the spy, the method returns undefined. + expect(obj.methodOne()).toBeUndefined(); + + // Real implementation has still not been called. + expect(methodOneRealCalls).toBe(0); }); it('supports resetting all spies', () => { const methodOneReturn = 10; - const methodTwoReturn = 20; + const methodTwoReturn = {}; + let methodOneRealCalls = 0; + let methodTwoRealCalls = 0; const obj = { methodOne() { + methodOneRealCalls++; return methodOneReturn; }, methodTwo() { + methodTwoRealCalls++; return methodTwoReturn; }, }; + // methodOne is spied on and mocked. moduleMocker.spyOn(obj, 'methodOne').mockReturnValue(100); - moduleMocker.spyOn(obj, 'methodTwo').mockReturnValue(200); + // methodTwo is spied on but not mocked. + moduleMocker.spyOn(obj, 'methodTwo'); // Return values are mocked. - expect(methodOneReturn).toBe(10); - expect(methodTwoReturn).toBe(20); expect(obj.methodOne()).toBe(100); - expect(obj.methodTwo()).toBe(200); + expect(obj.methodTwo()).toBe(methodTwoReturn); + // The real implementation has not been called when mocked. + expect(methodOneRealCalls).toBe(0); + + // But has for the unmocked spy. + expect(methodTwoRealCalls).toBe(1); + + // Both are mock functions. expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(true); expect(moduleMocker.isMockFunction(obj.methodTwo)).toBe(true); @@ -1602,11 +1638,16 @@ describe('moduleMocker', () => { expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(true); expect(moduleMocker.isMockFunction(obj.methodTwo)).toBe(true); - // After resetting all mocks, the methods return the original return value. - expect(methodOneReturn).toBe(10); - expect(methodTwoReturn).toBe(20); - expect(obj.methodOne()).toBe(10); - expect(obj.methodTwo()).toBe(20); + // After resetting all mocks, the methods are stubs returning undefined. + expect(obj.methodOne()).toBeUndefined(); + + // NB: It may not be desirable for reset to stub a spy that was never mocked - + // consider changing in a future major. + expect(obj.methodTwo()).toBeUndefined(); + + // Real functions have not been called any more times. + expect(methodOneRealCalls).toBe(0); + expect(methodTwoRealCalls).toBe(1); }); it('supports restoring a spy', () => { @@ -1654,32 +1695,25 @@ describe('moduleMocker', () => { const spy1 = moduleMocker.spyOn(obj, 'methodOne'); const spy2 = moduleMocker.spyOn(obj, 'methodTwo'); + // First, we call with the spies: both spies and both original functions + // should be called. obj.methodOne(); obj.methodTwo(); - - // Both spies and both original functions got called. expect(methodOneCalls).toBe(1); expect(methodTwoCalls).toBe(1); expect(spy1.mock.calls).toHaveLength(1); expect(spy2.mock.calls).toHaveLength(1); - expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(true); - expect(moduleMocker.isMockFunction(obj.methodTwo)).toBe(true); - moduleMocker.restoreAllMocks(); - // After restoring all mocks, the methods are not mock functions. - expect(moduleMocker.isMockFunction(obj.methodOne)).toBe(false); - expect(moduleMocker.isMockFunction(obj.methodTwo)).toBe(false); - + // Then, after resetting all mocks, we call methods again. Only the real + // methods should bump their count, not the spies. obj.methodOne(); obj.methodTwo(); - - // After restoring all mocks only the real methods bump their count, not the spies. expect(methodOneCalls).toBe(2); expect(methodTwoCalls).toBe(2); - expect(spy1.mock.calls).toHaveLength(0); - expect(spy2.mock.calls).toHaveLength(0); + expect(spy1.mock.calls).toHaveLength(1); + expect(spy2.mock.calls).toHaveLength(1); }); it('should work with getters', () => { @@ -1816,8 +1850,10 @@ describe('moduleMocker', () => { it('supports resetting a spy', () => { const methodOneReturn = 0; + let methodOneRealCalls = 0; const obj = { get methodOne() { + methodOneRealCalls++; return methodOneReturn; }, }; @@ -1827,14 +1863,13 @@ describe('moduleMocker', () => { .mockReturnValue(10); // Return value is mocked. - expect(methodOneReturn).toBe(0); expect(obj.methodOne).toBe(10); spy1.mockReset(); - // After resetting the spy, the method returns the original return value. - expect(methodOneReturn).toBe(0); - expect(obj.methodOne).toBe(0); + // After resetting the spy, the getter is a stub returning undefined + expect(obj.methodOne).toBeUndefined(); + expect(methodOneRealCalls).toBe(0); }); it('supports resetting all spies', () => { @@ -1860,11 +1895,9 @@ describe('moduleMocker', () => { moduleMocker.resetAllMocks(); - // After resetting all mocks, the methods return the original return value. - expect(methodOneReturn).toBe(10); - expect(methodTwoReturn).toBe(20); - expect(obj.methodOne).toBe(10); - expect(obj.methodTwo).toBe(20); + // After resetting all mocks, the methods are stubs + expect(obj.methodOne).toBeUndefined(); + expect(obj.methodTwo).toBeUndefined(); }); it('supports restoring a spy', () => { @@ -1913,10 +1946,10 @@ describe('moduleMocker', () => { const spy1 = moduleMocker.spyOn(obj, 'methodOne', 'get'); const spy2 = moduleMocker.spyOn(obj, 'methodTwo', 'get'); + // First, we call with the spies: both spies and both original functions + // should be called. obj.methodOne(); obj.methodTwo(); - - // Both spies and both original functions got called. expect(methodOneCalls).toBe(1); expect(methodTwoCalls).toBe(1); expect(spy1.mock.calls).toHaveLength(1); @@ -1924,14 +1957,14 @@ describe('moduleMocker', () => { moduleMocker.restoreAllMocks(); + // Then, after resetting all mocks, we call methods again. Only the real + // methods should bump their count, not the spies. obj.methodOne(); obj.methodTwo(); - - // After restoring all mocks only the real methods bump their count, not the spies. expect(methodOneCalls).toBe(2); expect(methodTwoCalls).toBe(2); - expect(spy1.mock.calls).toHaveLength(0); - expect(spy2.mock.calls).toHaveLength(0); + expect(spy1.mock.calls).toHaveLength(1); + expect(spy2.mock.calls).toHaveLength(1); }); it('should work with getters on the prototype chain', () => { @@ -2000,10 +2033,11 @@ describe('moduleMocker', () => { }); it('supports resetting a spy on the prototype chain', () => { - const methodOneReturn = 0; + let methodOneRealCalls = 0; const prototype = { get methodOne() { - return methodOneReturn; + methodOneRealCalls++; + return 1; }, }; const obj = Object.create(prototype, {}); @@ -2013,14 +2047,15 @@ describe('moduleMocker', () => { .mockReturnValue(10); // Return value is mocked. - expect(methodOneReturn).toBe(0); expect(obj.methodOne).toBe(10); spy1.mockReset(); - // After resetting the spy, the method returns the original return value. - expect(methodOneReturn).toBe(0); - expect(obj.methodOne).toBe(0); + // After resetting the spy, the method is a stub. + expect(obj.methodOne).toBeUndefined(); + + // The real implementation has not been used. + expect(methodOneRealCalls).toBe(0); }); it('supports resetting all spies on the prototype chain', () => { @@ -2040,18 +2075,14 @@ describe('moduleMocker', () => { moduleMocker.spyOn(obj, 'methodTwo', 'get').mockReturnValue(200); // Return values are mocked. - expect(methodOneReturn).toBe(10); - expect(methodTwoReturn).toBe(20); expect(obj.methodOne).toBe(100); expect(obj.methodTwo).toBe(200); moduleMocker.resetAllMocks(); - // After resetting all mocks, the methods return the original return value. - expect(methodOneReturn).toBe(10); - expect(methodTwoReturn).toBe(20); - expect(obj.methodOne).toBe(10); - expect(obj.methodTwo).toBe(20); + // After resetting all mocks, the methods are stubs + expect(obj.methodOne).toBeUndefined(); + expect(obj.methodTwo).toBeUndefined(); }); it('supports restoring a spy on the prototype chain', () => { @@ -2069,7 +2100,7 @@ describe('moduleMocker', () => { obj.methodOne(); - // The spy and the original function are called. + // The spy and the original function are called, because we have not mocked it. expect(methodOneCalls).toBe(1); expect(spy1.mock.calls).toHaveLength(1); @@ -2102,10 +2133,10 @@ describe('moduleMocker', () => { const spy1 = moduleMocker.spyOn(obj, 'methodOne', 'get'); const spy2 = moduleMocker.spyOn(obj, 'methodTwo', 'get'); + // First, we call with the spies: both spies and both original functions + // should be called. obj.methodOne(); obj.methodTwo(); - - // Both spies and both original functions got called. expect(methodOneCalls).toBe(1); expect(methodTwoCalls).toBe(1); expect(spy1.mock.calls).toHaveLength(1); @@ -2113,14 +2144,14 @@ describe('moduleMocker', () => { moduleMocker.restoreAllMocks(); + // Then, after resetting all mocks, we call methods again. Only the real + // methods should bump their count, not the spies. obj.methodOne(); obj.methodTwo(); - - // After restoring all mocks only the real methods bump their count, not the spies. expect(methodOneCalls).toBe(2); expect(methodTwoCalls).toBe(2); - expect(spy1.mock.calls).toHaveLength(0); - expect(spy2.mock.calls).toHaveLength(0); + expect(spy1.mock.calls).toHaveLength(1); + expect(spy2.mock.calls).toHaveLength(1); }); }); @@ -2159,7 +2190,7 @@ describe('moduleMocker', () => { expect(obj.property).toBe(1); }); - it('should allow mocking with value of different type', () => { + it('should allow mocking with value of different value', () => { const obj = { property: 1, }; diff --git a/packages/jest-mock/src/index.ts b/packages/jest-mock/src/index.ts index 8989c5320bf3..296934a66a85 100644 --- a/packages/jest-mock/src/index.ts +++ b/packages/jest-mock/src/index.ts @@ -254,8 +254,6 @@ type MockFunctionConfig = { specificMockImpls: Array; }; -type SpyState = {reset?: () => void; restore: () => void}; - const MOCK_CONSTRUCTOR_NAME = 'mockConstructor'; const FUNCTION_NAME_RESERVED_PATTERN = /[\s!-/:-@[-`{-~]/; @@ -504,7 +502,7 @@ export class ModuleMocker { private readonly _environmentGlobal: typeof globalThis; private _mockState: WeakMap; private _mockConfigRegistry: WeakMap; - private _spyState: Set; + private _spyState: Set<() => void>; private _invocationCallCounter: number; /** @@ -609,27 +607,27 @@ export class ModuleMocker { private _makeComponent>( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): T; private _makeComponent>( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): T; private _makeComponent( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): T; private _makeComponent( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): T; private _makeComponent( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): Mock; private _makeComponent( metadata: MockMetadata, - spyState?: SpyState, + restore?: () => void, ): Record | Array | RegExp | T | Mock | undefined { if (metadata.type === 'object') { return new this._environmentGlobal.Object(); @@ -750,8 +748,8 @@ export class ModuleMocker { f._isMockFunction = true; f.getMockImplementation = () => this._ensureMockConfig(f).mockImpl as T; - if (spyState != null) { - this._spyState.add(spyState); + if (typeof restore === 'function') { + this._spyState.add(restore); } this._mockState.set(f, this._defaultMockState()); @@ -772,22 +770,12 @@ export class ModuleMocker { f.mockReset = () => { f.mockClear(); this._mockConfigRegistry.delete(f); - - if (spyState != null) { - spyState.reset?.(); - } - return f; }; f.mockRestore = () => { - f.mockClear(); - this._mockConfigRegistry.delete(f); - - if (spyState != null) { - spyState.restore(); - this._spyState.delete(spyState); - } + f.mockReset(); + return restore ? restore() : undefined; }; f.mockReturnValueOnce = (value: ReturnType) => @@ -995,14 +983,14 @@ export class ModuleMocker { object: T, propertyKey: K, ): ReplacedPropertyRestorer | undefined { - for (const {restore} of this._spyState) { + for (const spyState of this._spyState) { if ( - 'object' in restore && - 'property' in restore && - restore.object === object && - restore.property === propertyKey + 'object' in spyState && + 'property' in spyState && + spyState.object === object && + spyState.property === propertyKey ) { - return restore as ReplacedPropertyRestorer; + return spyState as ReplacedPropertyRestorer; } } @@ -1118,15 +1106,6 @@ export class ModuleMocker { return fn; } - private _attachMockImplementation( - mock: Mock, - original: T, - ) { - mock.mockImplementation(function (this: unknown) { - return original.apply(this, arguments); - }); - } - spyOn< T extends object, K extends PropertyLikeKeys, @@ -1217,41 +1196,27 @@ export class ModuleMocker { if (descriptor && descriptor.get) { const originalGet = descriptor.get; - mock = this._makeComponent( - {type: 'function'}, - { - reset: () => { - this._attachMockImplementation(mock, original); - }, - restore: () => { - descriptor!.get = originalGet; - Object.defineProperty(object, methodKey, descriptor!); - }, - }, - ); + mock = this._makeComponent({type: 'function'}, () => { + descriptor!.get = originalGet; + Object.defineProperty(object, methodKey, descriptor!); + }); descriptor.get = () => mock; Object.defineProperty(object, methodKey, descriptor); } else { - mock = this._makeComponent( - {type: 'function'}, - { - reset: () => { - this._attachMockImplementation(mock, original); - }, - restore: () => { - if (isMethodOwner) { - object[methodKey] = original; - } else { - delete object[methodKey]; - } - }, - }, - ); - // @ts-expect-error: overriding original method with a mock + mock = this._makeComponent({type: 'function'}, () => { + if (isMethodOwner) { + object[methodKey] = original; + } else { + delete object[methodKey]; + } + }); + // @ts-expect-error overriding original method with a Mock object[methodKey] = mock; } - this._attachMockImplementation(mock, original); + mock.mockImplementation(function (this: unknown) { + return original.apply(this, arguments); + }); } return object[methodKey] as Mock; @@ -1311,24 +1276,18 @@ export class ModuleMocker { ); } - descriptor[accessType] = this._makeComponent( - {type: 'function'}, - { - reset: () => { - this._attachMockImplementation( - descriptor![accessType] as Mock, - original, - ); - }, - restore: () => { - // @ts-expect-error: overriding original method with a mock - descriptor![accessType] = original; - Object.defineProperty(object, propertyKey, descriptor!); - }, - }, - ); + descriptor[accessType] = this._makeComponent({type: 'function'}, () => { + // @ts-expect-error: mock is assignable + descriptor![accessType] = original; + Object.defineProperty(object, propertyKey, descriptor!); + }); - this._attachMockImplementation(descriptor[accessType] as Mock, original); + (descriptor[accessType] as Mock).mockImplementation(function ( + this: unknown, + ) { + // @ts-expect-error - wrong context + return original.apply(this, arguments); + }); } Object.defineProperty(object, propertyKey, descriptor); @@ -1434,7 +1393,7 @@ export class ModuleMocker { restore: () => { restore(); - this._spyState.delete({restore}); + this._spyState.delete(restore); }, }; @@ -1442,7 +1401,7 @@ export class ModuleMocker { restore.property = propertyKey; restore.replaced = replaced; - this._spyState.add({restore}); + this._spyState.add(restore); return replaced.replaceValue(value); } @@ -1452,15 +1411,12 @@ export class ModuleMocker { } resetAllMocks(): void { - this.clearAllMocks(); this._mockConfigRegistry = new WeakMap(); - this._spyState.forEach(spyState => spyState.reset?.()); + this._mockState = new WeakMap(); } restoreAllMocks(): void { - this.clearAllMocks(); - this._mockConfigRegistry = new WeakMap(); - this._spyState.forEach(spyState => spyState.restore()); + this._spyState.forEach(restore => restore()); this._spyState = new Set(); } diff --git a/website/versioned_docs/version-29.4/MockFunctionAPI.md b/website/versioned_docs/version-29.4/MockFunctionAPI.md index 5ffb11992b84..6282e1538f2d 100644 --- a/website/versioned_docs/version-29.4/MockFunctionAPI.md +++ b/website/versioned_docs/version-29.4/MockFunctionAPI.md @@ -128,9 +128,7 @@ Beware that `mockFn.mockClear()` will replace `mockFn.mock`, not just reset the ### `mockFn.mockReset()` -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. +Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also replaces the mock implementation with an empty function, returning `undefined`. The [`resetMocks`](configuration#resetmocks-boolean) configuration option is available to reset mocks automatically before each test. diff --git a/website/versioned_docs/version-29.5/MockFunctionAPI.md b/website/versioned_docs/version-29.5/MockFunctionAPI.md index 5ffb11992b84..6282e1538f2d 100644 --- a/website/versioned_docs/version-29.5/MockFunctionAPI.md +++ b/website/versioned_docs/version-29.5/MockFunctionAPI.md @@ -128,9 +128,7 @@ Beware that `mockFn.mockClear()` will replace `mockFn.mock`, not just reset the ### `mockFn.mockReset()` -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. +Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also replaces the mock implementation with an empty function, returning `undefined`. The [`resetMocks`](configuration#resetmocks-boolean) configuration option is available to reset mocks automatically before each test. diff --git a/website/versioned_docs/version-29.6/MockFunctionAPI.md b/website/versioned_docs/version-29.6/MockFunctionAPI.md index 5ffb11992b84..6282e1538f2d 100644 --- a/website/versioned_docs/version-29.6/MockFunctionAPI.md +++ b/website/versioned_docs/version-29.6/MockFunctionAPI.md @@ -128,9 +128,7 @@ Beware that `mockFn.mockClear()` will replace `mockFn.mock`, not just reset the ### `mockFn.mockReset()` -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. +Does everything that [`mockFn.mockClear()`](#mockfnmockclear) does, and also replaces the mock implementation with an empty function, returning `undefined`. The [`resetMocks`](configuration#resetmocks-boolean) configuration option is available to reset mocks automatically before each test.