From 922aff2eebecfbb5778d7e5cd3b70be50d4de876 Mon Sep 17 00:00:00 2001 From: Tom Mrazauskas Date: Wed, 19 Apr 2023 16:42:46 +0300 Subject: [PATCH 1/3] fix(jest-mock): improve user input validation and error messages of `spyOn` and `replaceProperty` methods --- .../jest-mock/src/__tests__/index.test.ts | 259 ++++++++++++++---- packages/jest-mock/src/index.ts | 82 +++--- 2 files changed, 257 insertions(+), 84 deletions(-) diff --git a/packages/jest-mock/src/__tests__/index.test.ts b/packages/jest-mock/src/__tests__/index.test.ts index 8f178c896b8d..ef072eafe4e2 100644 --- a/packages/jest-mock/src/__tests__/index.test.ts +++ b/packages/jest-mock/src/__tests__/index.test.ts @@ -1304,20 +1304,164 @@ describe('moduleMocker', () => { expect(spy).not.toHaveBeenCalled(); }); - it('should throw on invalid input', () => { - expect(() => { - moduleMocker.spyOn(null, 'method'); - }).toThrow('spyOn could not find an object to spy on for method'); - expect(() => { - moduleMocker.spyOn({}, 'method'); - }).toThrow( - "Cannot spy on the method property because it is not a function; undefined given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", - ); - expect(() => { - moduleMocker.spyOn({method: 10}, 'method'); - }).toThrow( - "Cannot spy on the method property because it is not a function; number given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", + describe('should throw', () => { + it.each` + value | type + ${'foo'} | ${'string'} + ${1} | ${'number'} + ${NaN} | ${'number'} + ${1n} | ${'bigint'} + ${Symbol()} | ${'symbol'} + ${true} | ${'boolean'} + ${false} | ${'boolean'} + ${undefined} | ${'undefined'} + ${null} | ${'null'} + `( + 'when primitive value $value is provided instead of an object', + ({value, type}) => { + expect(() => { + moduleMocker.spyOn(value, 'method'); + }).toThrow(`Cannot use spyOn on a primitive value; ${type} given`); + }, ); + + it('when property name is not provided', () => { + expect(() => { + moduleMocker.spyOn({}, null); + }).toThrow('No property name supplied'); + }); + + it('when property does not exist', () => { + expect(() => { + moduleMocker.spyOn({}, 'doesNotExist'); + }).toThrow( + 'Property `doesNotExist` does not exist in the provided object', + ); + }); + + it('when getter does not exist', () => { + expect(() => { + moduleMocker.spyOn({}, 'missingGet', 'get'); + }).toThrow( + 'Property `missingGet` does not exist in the provided object', + ); + }); + + it('when setter does not exist', () => { + expect(() => { + moduleMocker.spyOn({}, 'missingSet', 'set'); + }).toThrow( + 'Property `missingSet` does not exist in the provided object', + ); + }); + + it('when getter is not configurable', () => { + expect(() => { + const obj = {}; + + Object.defineProperty(obj, 'property', { + configurable: false, + get() { + return 1; + }, + }); + + moduleMocker.spyOn(obj, 'property', 'get'); + }).toThrow('Property `property` is not declared configurable'); + }); + + it('when setter is not configurable', () => { + expect(() => { + const obj = {}; + let value = 38; + + Object.defineProperty(obj, 'property', { + configurable: false, + get() { + return value; + }, + set(newValue) { + value = newValue; + }, + }); + + moduleMocker.spyOn(obj, 'property', 'set'); + }).toThrow('Property `property` is not declared configurable'); + }); + + it('when property does not have access type get', () => { + expect(() => { + const obj = {}; + let value = 38; + + // eslint-disable-next-line accessor-pairs + Object.defineProperty(obj, 'property', { + configurable: true, + set(newValue) { + value = newValue; + }, + }); + + moduleMocker.spyOn(obj, 'property', 'get'); + }).toThrow('Property `property` does not have access type get'); + }); + + it('when property does not have access type set', () => { + expect(() => { + const obj = {}; + + Object.defineProperty(obj, 'property', { + configurable: true, + get() { + return 1; + }, + }); + + moduleMocker.spyOn(obj, 'property', 'set'); + }).toThrow('Property `property` does not have access type set'); + }); + + it('when trying to spy on a non function property', () => { + expect(() => { + moduleMocker.spyOn({property: 123}, 'property'); + }).toThrow( + "Cannot spy on the `property` property because it is not a function; number given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'property', value)` instead.", + ); + }); + }); + + it('supports spying on a method named `0`', () => { + let haveBeenCalled = false; + const obj = { + 0: () => { + haveBeenCalled = true; + }, + }; + + const spy = moduleMocker.spyOn(obj, 0); + obj[0].call(null); + + expect(haveBeenCalled).toBe(true); + expect(spy).toHaveBeenCalled(); + }); + + it('supports spying on a method which is defined on a function', () => { + let haveBeenCalled = false; + const obj = () => true; + + Object.defineProperty(obj, 'method', { + configurable: true, + value: () => { + haveBeenCalled = true; + }, + writable: true, + }); + + const spy = moduleMocker.spyOn(obj, 'method'); + obj['method'].call(null); + + expect(haveBeenCalled).toBe(true); + expect(spy).toHaveBeenCalled(); }); it('supports clearing a spy', () => { @@ -1642,16 +1786,14 @@ describe('moduleMocker', () => { it('should throw on invalid input', () => { expect(() => { moduleMocker.spyOn(null, 'method'); - }).toThrow('spyOn could not find an object to spy on for method'); + }).toThrow('Cannot use spyOn on a primitive value; null given'); expect(() => { moduleMocker.spyOn({}, 'method'); - }).toThrow( - "Cannot spy on the method property because it is not a function; undefined given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", - ); + }).toThrow('Property `method` does not exist in the provided object'); expect(() => { moduleMocker.spyOn({method: 10}, 'method'); }).toThrow( - "Cannot spy on the method property because it is not a function; number given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", + "Cannot spy on the `method` property because it is not a function; number given instead. If you are trying to mock a property, use `jest.replaceProperty(object, 'method', value)` instead.", ); }); @@ -2018,34 +2160,23 @@ describe('moduleMocker', () => { describe('should throw', () => { it.each` - value - ${null} - ${undefined} - `('when $value is provided instead of an object', ({value}) => { - expect(() => { - moduleMocker.replaceProperty(value, 'property', 1); - }).toThrow( - 'replaceProperty could not find an object on which to replace property', - ); - }); - - it.each` - value | type - ${'foo'} | ${'string'} - ${1} | ${'number'} - ${NaN} | ${'number'} - ${1n} | ${'bigint'} - ${Symbol()} | ${'symbol'} - ${true} | ${'boolean'} - ${false} | ${'boolean'} - ${() => {}} | ${'function'} + value | type + ${'foo'} | ${'string'} + ${1} | ${'number'} + ${NaN} | ${'number'} + ${1n} | ${'bigint'} + ${Symbol()} | ${'symbol'} + ${true} | ${'boolean'} + ${false} | ${'boolean'} + ${undefined} | ${'undefined'} + ${null} | ${'null'} `( 'when primitive value $value is provided instead of an object', ({value, type}) => { expect(() => { moduleMocker.replaceProperty(value, 'property', 1); }).toThrow( - `Cannot mock property on a non-object value; ${type} given`, + `Cannot use replaceProperty on a primitive value; ${type} given`, ); }, ); @@ -2056,10 +2187,12 @@ describe('moduleMocker', () => { }).toThrow('No property name supplied'); }); - it('when property is not defined', () => { + it('when property does not exist', () => { expect(() => { moduleMocker.replaceProperty({}, 'doesNotExist', 1); - }).toThrow('doesNotExist property does not exist'); + }).toThrow( + 'Property `doesNotExist` does not exist in the provided object', + ); }); it('when property is not configurable', () => { @@ -2073,18 +2206,18 @@ describe('moduleMocker', () => { }); moduleMocker.replaceProperty(obj, 'property', 2); - }).toThrow('property is not declared configurable'); + }).toThrow('Property `property` is not declared configurable'); }); - it('when trying to mock a method', () => { + it('when trying to replace a method', () => { expect(() => { moduleMocker.replaceProperty({method: () => {}}, 'method', () => {}); }).toThrow( - "Cannot mock the method property because it is a function. Use `jest.spyOn(object, 'method')` instead.", + "Cannot replace the `method` property because it is a function. Use `jest.spyOn(object, 'method')` instead.", ); }); - it('when mocking a getter', () => { + it('when trying to replace a getter', () => { const obj = { get getter() { return 1; @@ -2093,10 +2226,12 @@ describe('moduleMocker', () => { expect(() => { moduleMocker.replaceProperty(obj, 'getter', 1); - }).toThrow('Cannot mock the getter property because it has a getter'); + }).toThrow( + 'Cannot replace the `getter` property because it has a getter', + ); }); - it('when mocking a setter', () => { + it('when trying to replace a setter', () => { const obj = { // eslint-disable-next-line accessor-pairs set setter(_value: number) {}, @@ -2104,10 +2239,36 @@ describe('moduleMocker', () => { expect(() => { moduleMocker.replaceProperty(obj, 'setter', 1); - }).toThrow('Cannot mock the setter property because it has a setter'); + }).toThrow( + 'Cannot replace the `setter` property because it has a setter', + ); }); }); + it('supports replacing a property named `0`', () => { + const obj = { + 0: 'zero', + }; + + moduleMocker.replaceProperty(obj, 0, 'null'); + + expect(obj[0]).toBe('null'); + }); + + it('supports replacing a property which is defined on a function', () => { + const obj = () => true; + + Object.defineProperty(obj, 'property', { + configurable: true, + value: 'abc', + writable: true, + }); + + moduleMocker.replaceProperty(obj, 'property', 'def'); + + expect(obj['property']).toBe('def'); + }); + it('should work for property from prototype chain', () => { const parent = {property: 'abcd'}; const child = Object.create(parent); diff --git a/packages/jest-mock/src/index.ts b/packages/jest-mock/src/index.ts index 6801429f1626..8989c5320bf3 100644 --- a/packages/jest-mock/src/index.ts +++ b/packages/jest-mock/src/index.ts @@ -1156,19 +1156,16 @@ export class ModuleMocker { methodKey: keyof T, accessType?: 'get' | 'set', ): MockInstance { - if (typeof object !== 'object' && typeof object !== 'function') { + if ( + object == null || + (typeof object !== 'object' && typeof object !== 'function') + ) { throw new Error( `Cannot use spyOn on a primitive value; ${this._typeOf(object)} given`, ); } - if (!object) { - throw new Error( - `spyOn could not find an object to spy on for ${String(methodKey)}`, - ); - } - - if (!methodKey) { + if (methodKey == null) { throw new Error('No property name supplied'); } @@ -1178,12 +1175,20 @@ export class ModuleMocker { const original = object[methodKey]; + if (!original) { + throw new Error( + `Property \`${String( + methodKey, + )}\` does not exist in the provided object`, + ); + } + if (!this.isMockFunction(original)) { if (typeof original !== 'function') { throw new Error( - `Cannot spy on the ${String( + `Cannot spy on the \`${String( methodKey, - )} property because it is not a function; ${this._typeOf( + )}\` property because it is not a function; ${this._typeOf( original, )} given instead.${ typeof original !== 'object' @@ -1266,18 +1271,24 @@ export class ModuleMocker { } if (!descriptor) { - throw new Error(`${String(propertyKey)} property does not exist`); + throw new Error( + `Property \`${String( + propertyKey, + )}\` does not exist in the provided object`, + ); } if (!descriptor.configurable) { - throw new Error(`${String(propertyKey)} is not declared configurable`); + throw new Error( + `Property \`${String(propertyKey)}\` is not declared configurable`, + ); } if (!descriptor[accessType]) { throw new Error( - `Property ${String( + `Property \`${String( propertyKey, - )} does not have access type ${accessType}`, + )}\` does not have access type ${accessType}`, ); } @@ -1329,26 +1340,21 @@ export class ModuleMocker { propertyKey: K, value: T[K], ): Replaced { - if (object === undefined || object == null) { + if ( + object == null || + (typeof object !== 'object' && typeof object !== 'function') + ) { throw new Error( - `replaceProperty could not find an object on which to replace ${String( - propertyKey, - )}`, + `Cannot use replaceProperty on a primitive value; ${this._typeOf( + object, + )} given`, ); } - if (propertyKey === undefined || propertyKey === null) { + if (propertyKey == null) { throw new Error('No property name supplied'); } - if (typeof object !== 'object') { - throw new Error( - `Cannot mock property on a non-object value; ${this._typeOf( - object, - )} given`, - ); - } - let descriptor = Object.getOwnPropertyDescriptor(object, propertyKey); let proto = Object.getPrototypeOf(object); while (!descriptor && proto !== null) { @@ -1356,17 +1362,23 @@ export class ModuleMocker { proto = Object.getPrototypeOf(proto); } if (!descriptor) { - throw new Error(`${String(propertyKey)} property does not exist`); + throw new Error( + `Property \`${String( + propertyKey, + )}\` does not exist in the provided object`, + ); } if (!descriptor.configurable) { - throw new Error(`${String(propertyKey)} is not declared configurable`); + throw new Error( + `Property \`${String(propertyKey)}\` is not declared configurable`, + ); } if (descriptor.get !== undefined) { throw new Error( - `Cannot mock the ${String( + `Cannot replace the \`${String( propertyKey, - )} property because it has a getter. Use \`jest.spyOn(object, '${String( + )}\` property because it has a getter. Use \`jest.spyOn(object, '${String( propertyKey, )}', 'get').mockReturnValue(value)\` instead.`, ); @@ -1374,9 +1386,9 @@ export class ModuleMocker { if (descriptor.set !== undefined) { throw new Error( - `Cannot mock the ${String( + `Cannot replace the \`${String( propertyKey, - )} property because it has a setter. Use \`jest.spyOn(object, '${String( + )}\` property because it has a setter. Use \`jest.spyOn(object, '${String( propertyKey, )}', 'set').mockReturnValue(value)\` instead.`, ); @@ -1384,9 +1396,9 @@ export class ModuleMocker { if (typeof descriptor.value === 'function') { throw new Error( - `Cannot mock the ${String( + `Cannot replace the \`${String( propertyKey, - )} property because it is a function. Use \`jest.spyOn(object, '${String( + )}\` property because it is a function. Use \`jest.spyOn(object, '${String( propertyKey, )}')\` instead.`, ); From 75bacf33c9a200581e1bd67cc4280bbb6786f030 Mon Sep 17 00:00:00 2001 From: Tom Mrazauskas Date: Wed, 19 Apr 2023 17:12:56 +0300 Subject: [PATCH 2/3] symbol-keyed cases --- .../jest-mock/src/__tests__/index.test.ts | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/packages/jest-mock/src/__tests__/index.test.ts b/packages/jest-mock/src/__tests__/index.test.ts index ef072eafe4e2..7d08d17203df 100644 --- a/packages/jest-mock/src/__tests__/index.test.ts +++ b/packages/jest-mock/src/__tests__/index.test.ts @@ -1445,6 +1445,23 @@ describe('moduleMocker', () => { expect(spy).toHaveBeenCalled(); }); + it('supports spying on a symbol-keyed method', () => { + const k = Symbol(); + + let haveBeenCalled = false; + const obj = { + [k]: () => { + haveBeenCalled = true; + }, + }; + + const spy = moduleMocker.spyOn(obj, k); + obj[k].call(null); + + expect(haveBeenCalled).toBe(true); + expect(spy).toHaveBeenCalled(); + }); + it('supports spying on a method which is defined on a function', () => { let haveBeenCalled = false; const obj = () => true; @@ -2255,6 +2272,18 @@ describe('moduleMocker', () => { expect(obj[0]).toBe('null'); }); + it('supports replacing a symbol-keyed property', () => { + const k = Symbol(); + + const obj = { + [k]: 'zero', + }; + + moduleMocker.replaceProperty(obj, k, 'null'); + + expect(obj[k]).toBe('null'); + }); + it('supports replacing a property which is defined on a function', () => { const obj = () => true; From eba30e7f799764252b6a42d65f21f073e82d40ba Mon Sep 17 00:00:00 2001 From: Tom Mrazauskas Date: Wed, 19 Apr 2023 17:14:21 +0300 Subject: [PATCH 3/3] add change log entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 309e3d342619..22bb50dbacf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - `[jest-environment-jsdom, jest-environment-node]` Fix assignment of `customExportConditions` via `testEnvironmentOptions` when custom env subclass defines a default value ([#13989](https://github.com/facebook/jest/pull/13989)) - `[jest-matcher-utils]` Fix copying value of inherited getters ([#14007](https://github.com/facebook/jest/pull/14007)) - `[jest-mock]` Tweak typings to allow `jest.replaceProperty()` replace methods ([#14008](https://github.com/facebook/jest/pull/14008)) +- `[jest-mock]` Improve user input validation and error messages of `spyOn` and `replaceProperty` methods ([#14087](https://github.com/facebook/jest/pull/14087)) - `[jest-snapshot]` Fix a potential bug when not using prettier and improve performance ([#14036](https://github.com/facebook/jest/pull/14036)) - `[@jest/transform]` Do not instrument `.json` modules ([#14048](https://github.com/facebook/jest/pull/14048))