diff --git a/CHANGELOG.md b/CHANGELOG.md index fc625ea0..6c68fe81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,22 @@ condition now gets the browser build, which can't replace modules. That includes Jest's jsdom environment with `--experimental-vm-modules`, and Node run with `--conditions=browser`. `require('testdouble')` is unaffected +* Fix getter properties coming out of `td.imitate` as `undefined`. This affected + `td.replace(path)` (including through testdouble-jest), `td.object`, + `td.constructor` and `td.imitate`. Most visibly, every export of a module + compiled by SWC, and re-exports compiled by Babel or `tsc`, couldn't be + stubbed [#526](https://github.com/testdouble/testdouble.js/issues/526) +* Fix `td.replace(object, 'property')` throwing `Cannot set property ... which + has only a getter` when the property is a getter with no setter. It now + replaces the getter, and `td.reset()` restores the original getter. A getter + that can't be redefined (as SWC defines exports) gets an error that suggests + `td.replace('path/to/module')` instead + [#526](https://github.com/testdouble/testdouble.js/issues/526) +* **Behavior change:** when imitating an object, testdouble now calls its + getters and imitates what they return, instead of using `undefined`. Any side + effects of those getters now run. Getters that throw, getters on prototypes + (like those `td.constructor` reads), and getters nested more than 3 deep still + give `undefined` ## 3.23.0 diff --git a/examples/babel/lib/calculator.js b/examples/babel/lib/calculator.js index ffc504eb..9468a6d0 100644 --- a/examples/babel/lib/calculator.js +++ b/examples/babel/lib/calculator.js @@ -2,6 +2,7 @@ import Adder from './adder' import Subtractor from './subtractor' import Multiplier from './multiplier' import divider from './divider' +import { square } from './operations' export default class Calculator { calculate(operation, ...operands) { @@ -16,6 +17,8 @@ export default class Calculator { divider.numerator.set(operands[0]) divider.otherOptions.divisor.setThatToo(operands[1]) return new divider.ohAndAlso.Executor().execute() + case 'square': + return square(operands[0]) } } } diff --git a/examples/babel/lib/operations.js b/examples/babel/lib/operations.js new file mode 100644 index 00000000..1d14498e --- /dev/null +++ b/examples/babel/lib/operations.js @@ -0,0 +1,2 @@ +// Babel compiles re-exports like this one to getters +export { square } from './squarer' diff --git a/examples/babel/lib/squarer.js b/examples/babel/lib/squarer.js new file mode 100644 index 00000000..f27ce073 --- /dev/null +++ b/examples/babel/lib/squarer.js @@ -0,0 +1,3 @@ +export function square (n) { + throw 'not implemented!' +} diff --git a/examples/babel/test/lib/calculator-test.js b/examples/babel/test/lib/calculator-test.js index fcd3a0ae..c0a5b570 100644 --- a/examples/babel/test/lib/calculator-test.js +++ b/examples/babel/test/lib/calculator-test.js @@ -2,7 +2,7 @@ import assert from 'assert' import { describe, it, beforeEach, afterEach } from 'node:test' import { td, setup, teardown } from '../helper' -let subject, Adder, Subtractor, Multiplier, divider, result +let subject, Adder, Subtractor, Multiplier, divider, operations, result describe('Calculator', () => { describe('#calculate', () => { beforeEach(() => { @@ -12,6 +12,7 @@ describe('Calculator', () => { Subtractor = td.replace('../../lib/subtractor') Multiplier = td.replace('../../lib/multiplier').default divider = td.replace('../../lib/divider').default + operations = td.replace('../../lib/operations') const Calculator = require('../../lib/calculator').default subject = new Calculator() }) @@ -55,5 +56,14 @@ describe('Calculator', () => { td.verify(divider.otherOptions.divisor.setThatToo(2)) assert.equal(result, 'lol') }) + + it('delegates to a squarer re-exported from a barrel file', () => { + assert.equal(td.explain(operations.square).isTestDouble, true) + td.when(operations.square(3)).thenReturn('squared math!') + + result = subject.calculate('square', 3) + + assert.equal(result, 'squared math!') + }) }) }) diff --git a/src/imitate/overwrite-children/copy-props.js b/src/imitate/overwrite-children/copy-props.js index f25279c9..75d7764b 100644 --- a/src/imitate/overwrite-children/copy-props.js +++ b/src/imitate/overwrite-children/copy-props.js @@ -1,17 +1,22 @@ import _ from '../../wrap/lodash' -export default (target, props, visitor) => { +// Getters can return fresh objects with getters of their own, so cap how +// deeply getter values are expanded to keep imitation finite +const MAX_GETTER_DEPTH = 3 +let getterDepth = 0 + +export default (target, props, visitor, original) => { Object.defineProperties(target, _.transform(props, (acc, descriptor, name) => { if (propOnTargetAndNotWritable(target, name, descriptor)) { if (name === 'prototype') { // Functions' prototype is not configurable but is assignable: - target.prototype = newValue(name, descriptor.value, visitor) + target.prototype = valueFor(name, descriptor, original, visitor) } } else { acc[name] = { configurable: true, writable: true, - value: newValue(name, descriptor.value, visitor), + value: valueFor(name, descriptor, original, visitor), enumerable: descriptor.enumerable } } @@ -26,6 +31,30 @@ const propOnTargetAndNotWritable = (target, name, originalDescriptor) => { } } +// Accessor properties have no `value`, so call the getter on the original +// object and imitate what it returns. Fall back to `undefined` whenever that +// isn't possible or safe +const valueFor = (name, descriptor, original, visitor) => { + if (!('get' in descriptor)) return newValue(name, descriptor.value, visitor) + if (!descriptor.get || original == null || getterDepth >= MAX_GETTER_DEPTH) return undefined + + getterDepth++ + try { + if (isPrototype(original)) return undefined + return newValue(name, descriptor.get.call(original), visitor) + } catch (e) { + return undefined + } finally { + getterDepth-- + } +} + +// A prototype has no instance to act as `this`, so its getters can't be run +const isPrototype = (thing) => + Object.prototype.hasOwnProperty.call(thing, 'constructor') && + _.isFunction(thing.constructor) && + thing.constructor.prototype === thing + const newValue = (name, value, visitor) => { return visitor ? visitor(name, value) : value } diff --git a/src/imitate/overwrite-children/index.js b/src/imitate/overwrite-children/index.js index abb6ec05..de37e45f 100644 --- a/src/imitate/overwrite-children/index.js +++ b/src/imitate/overwrite-children/index.js @@ -15,7 +15,6 @@ export default (original, target, overwriteChild) => { } else { copyProps(target, gatherProps(original), (name, originalValue) => chainPrototype(original, target, name, originalValue, - overwriteChild(originalValue, `.${name}`)) - ) + overwriteChild(originalValue, `.${name}`)), original) } } diff --git a/src/replace/property.js b/src/replace/property.js index 7a890eec..2c5c8b59 100644 --- a/src/replace/property.js +++ b/src/replace/property.js @@ -10,11 +10,23 @@ export default function (object, property, manualReplacement) { Object.prototype.hasOwnProperty.call(object, property) if (isManual || realThingExists) { + const getter = findGetterOnly(object, property) + if (getter && getter.isOwn && !getter.descriptor.configurable) { + return log.error('td.replace', `"${property}" is defined with a getter that cannot be redefined. Compilers like SWC and Babel define ES module exports this way. To replace a module's export, replace the whole module with td.replace('path/to/module') instead.`) + } const realThing = object[property] return _.tap(getFake(isManual, property, manualReplacement, realThing), (fakeThing) => { - object[property] = fakeThing + if (getter) { + defineValue(object, property, fakeThing, getter.descriptor.enumerable) + } else { + object[property] = fakeThing + } reset.onNextReset(() => { - if (realThingExists) { + if (getter && getter.isOwn) { + Object.defineProperty(object, property, getter.descriptor) + } else if (getter) { + delete object[property] + } else if (realThingExists) { object[property] = realThing } else { delete object[property] @@ -26,6 +38,30 @@ export default function (object, property, manualReplacement) { } } +// Assigning to a getter with no setter throws, so find any such accessor +// (on the object or up its prototype chain) to replace it by definition +const findGetterOnly = (object, property) => { + let thing = object + while (thing != null) { + const descriptor = Object.getOwnPropertyDescriptor(thing, property) + if (descriptor) { + return descriptor.get && !descriptor.set + ? { descriptor, isOwn: thing === object } + : undefined + } + thing = Object.getPrototypeOf(thing) + } +} + +const defineValue = (object, property, value, enumerable) => { + Object.defineProperty(object, property, { + configurable: true, + writable: true, + enumerable, + value + }) +} + const getFake = (isManual, property, manualReplacement, realThing) => { if (isManual) { warnIfTypeMismatch(property, manualReplacement, realThing) diff --git a/test/safe/constructor.test.js b/test/safe/constructor.test.js index 63327c52..0093d6c0 100644 --- a/test/safe/constructor.test.js +++ b/test/safe/constructor.test.js @@ -115,5 +115,19 @@ module.exports = { const fakeFunc = td.constructor(boringFunc) assert._isEqual(td.explain(fakeFunc.foo).isTestDouble, true) + }, + 'getters on the prototype' () { + let called = false + class Foo { + get bar () { + called = true + return 'real bar' + } + } + + FakeConstructor = td.constructor(Foo) + + assert._isEqual(new FakeConstructor().bar, undefined) + assert._isEqual(called, false) } } diff --git a/test/safe/fixtures/getter-exports.js b/test/safe/fixtures/getter-exports.js new file mode 100644 index 00000000..9f1b265e --- /dev/null +++ b/test/safe/fixtures/getter-exports.js @@ -0,0 +1,22 @@ +// CommonJS in the shape SWC compiles ES module exports to: every export is a +// non-configurable getter +Object.defineProperty(exports, '__esModule', { value: true }) + +function doTheThing () { + return 'real thing' +} + +const helpers = { + beep () { return 'real beep' } +} + +Object.defineProperty(exports, 'doTheThing', { + enumerable: true, + get: function () { return doTheThing } +}) + +// A re-export, which Babel and tsc also compile to a getter +Object.defineProperty(exports, 'beep', { + enumerable: true, + get: function () { return helpers.beep } +}) diff --git a/test/safe/imitate.test.js b/test/safe/imitate.test.js index ecbdbe5f..01b90a49 100644 --- a/test/safe/imitate.test.js +++ b/test/safe/imitate.test.js @@ -144,5 +144,30 @@ module.exports = { assert._isEqual(td.explain(td.imitate(foo)).name, '(anonymous function)') assert._isEqual(td.explain(td.imitate(foo).bar).name, '.bar') } + }, + 'imitating getters': { + 'imitates the value a getter returns' () { + const result = td.imitate({ + get answer () { return 42 }, + get fn () { return function doIt () {} } + }) + + assert._isEqual(result.answer, 42) + assert._isEqual(td.explain(result.fn).isTestDouble, true) + }, + 'stops expanding getters that return fresh objects forever' () { + const makeLink = () => ({ get next () { return makeLink() } }) + + const result = td.imitate(makeLink()) + + assert._isEqual(typeof result.next.next.next, 'object') + assert._isEqual(result.next.next.next.next, undefined) + }, + 'uses undefined when the getter\'s value can\'t be imitated' () { + const result = td.imitate({ get p () { return process } }) + + assert._isEqual(Object.prototype.hasOwnProperty.call(result, 'p'), true) + assert._isEqual(result.p, undefined) + } } } diff --git a/test/safe/object.test.js b/test/safe/object.test.js index a5a7e5ee..0006741f 100644 --- a/test/safe/object.test.js +++ b/test/safe/object.test.js @@ -123,5 +123,37 @@ module.exports = { assert._isEqual(testDouble.something(), undefined) assert._isEqual(testDouble.different.something(), undefined) } + }, + 'getters on class instances': { + 'reads the getter\'s value from the instance' () { + class Foo { + constructor () { this.k = 'abc' } + get bar () { return this.k.length } + get broken () { throw new Error('nope') } + } + + testDouble = td.object(new Foo()) + + assert._isEqual(testDouble.bar, 3) + assert._isEqual(testDouble.broken, undefined) + }, + 'turns a getter that returns a function into a test double' () { + class Foo { + get doIt () { return function doIt () {} } + } + + testDouble = td.object(new Foo()) + + assert._isEqual(td.explain(testDouble.doIt).isTestDouble, true) + }, + 'finishes when a getter returns a fresh instance of its own class' () { + class A { + get kid () { return new A() } + } + + testDouble = td.object(new A()) + + assert._isEqual(typeof testDouble.kid, 'object') + } } } diff --git a/test/safe/replace.test.js b/test/safe/replace.test.js index f76003ea..5760d659 100644 --- a/test/safe/replace.test.js +++ b/test/safe/replace.test.js @@ -149,6 +149,134 @@ module.exports = { assert._isEqual(Object.prototype.hasOwnProperty.call(dependency, 'notAThing'), false) } }, + 'Replacing a property defined with a getter': { + 'own and configurable (tsc re-exports)': { + beforeEach () { + Object.defineProperty(dependency, 'beep', { + configurable: true, + enumerable: true, + get: () => () => 'og beep' + }) + }, + 'using automatic replacement' () { + const ogDescriptor = Object.getOwnPropertyDescriptor(dependency, 'beep') + + const double = td.replace(dependency, 'beep') + + assert._isEqual(td.explain(double).isTestDouble, true) + assert._isEqual(dependency.beep, double) + + // phase 2: reset puts the getter itself back + td.reset() + assert._isEqual(Object.getOwnPropertyDescriptor(dependency, 'beep'), ogDescriptor) + assert._isEqual(dependency.beep(), 'og beep') + }, + 'with manual replacement' () { + const ogDescriptor = Object.getOwnPropertyDescriptor(dependency, 'beep') + + const fake = td.replace(dependency, 'beep', () => 'fake beep') + + assert._isEqual(dependency.beep, fake) + + // phase 2: reset puts the getter itself back + td.reset() + assert._isEqual(Object.getOwnPropertyDescriptor(dependency, 'beep'), ogDescriptor) + } + }, + 'inherited from a class': { + beforeEach () { + class Horn { + get beep () { return () => 'og beep' } + } + dependency = new Horn() + }, + 'using automatic replacement' () { + const double = td.replace(dependency, 'beep') + + assert._isEqual(td.explain(double).isTestDouble, true) + assert._isEqual(dependency.beep, double) + assert._isEqual(Object.prototype.hasOwnProperty.call(dependency, 'beep'), true) + + // phase 2: reset uncovers the inherited getter again + td.reset() + assert._isEqual(Object.prototype.hasOwnProperty.call(dependency, 'beep'), false) + assert._isEqual(dependency.beep(), 'og beep') + }, + 'with manual replacement' () { + const fake = td.replace(dependency, 'beep', () => 'fake beep') + + assert._isEqual(dependency.beep, fake) + + // phase 2: reset uncovers the inherited getter again + td.reset() + assert._isEqual(Object.prototype.hasOwnProperty.call(dependency, 'beep'), false) + assert._isEqual(dependency.beep(), 'og beep') + } + }, + 'own and non-configurable (SWC exports)': { + beforeEach () { + Object.defineProperty(dependency, 'beep', { + enumerable: true, + get: () => () => 'og beep' + }) + }, + 'using automatic replacement' () { + const ogDescriptor = Object.getOwnPropertyDescriptor(dependency, 'beep') + + try { + td.replace(dependency, 'beep') + assert.fail('should error!') + } catch (e) { + assert._isEqual(e.message, 'Error: testdouble.js - td.replace - "beep" is defined with a getter that cannot be redefined. Compilers like SWC and Babel define ES module exports this way. To replace a module\'s export, replace the whole module with td.replace(\'path/to/module\') instead.') + } + assert._isEqual(Object.getOwnPropertyDescriptor(dependency, 'beep'), ogDescriptor) + }, + 'with manual replacement' () { + const ogDescriptor = Object.getOwnPropertyDescriptor(dependency, 'beep') + + try { + td.replace(dependency, 'beep', () => 'fake beep') + assert.fail('should error!') + } catch (e) { + assert._isEqual(e.message, 'Error: testdouble.js - td.replace - "beep" is defined with a getter that cannot be redefined. Compilers like SWC and Babel define ES module exports this way. To replace a module\'s export, replace the whole module with td.replace(\'path/to/module\') instead.') + } + assert._isEqual(Object.getOwnPropertyDescriptor(dependency, 'beep'), ogDescriptor) + } + }, + 'with a setter': { + beforeEach () { + let beep = () => 'og beep' + Object.defineProperty(dependency, 'beep', { + configurable: true, + enumerable: true, + get: () => beep, + set: (value) => { beep = value } + }) + }, + 'using automatic replacement' () { + const ogDescriptor = Object.getOwnPropertyDescriptor(dependency, 'beep') + + const double = td.replace(dependency, 'beep') + + assert._isEqual(td.explain(double).isTestDouble, true) + assert._isEqual(dependency.beep, double) + assert._isEqual(Object.getOwnPropertyDescriptor(dependency, 'beep'), ogDescriptor) + + // phase 2: reset restores through the setter + td.reset() + assert._isEqual(dependency.beep(), 'og beep') + }, + 'with manual replacement' () { + const fake = td.replace(dependency, 'beep', () => 'fake beep') + + assert._isEqual(dependency.beep, fake) + + // phase 2: reset restores through the setter + td.reset() + assert._isEqual(dependency.beep(), 'og beep') + } + } + }, 'Manually specifying the override': { beforeEach () { ogWarn = console.warn @@ -241,6 +369,14 @@ module.exports = { assert._isEqual(car.isASpeed('a speed'), true) }, + 'a module whose exports are getters' () { + const getterExports = td.replace('./fixtures/getter-exports') + + assert._isEqual(td.explain(getterExports.doTheThing).isTestDouble, true) + assert._isEqual(td.explain(getterExports.beep).isTestDouble, true) + td.when(getterExports.doTheThing()).thenReturn(123) + assert._isEqual(require('./fixtures/getter-exports').doTheThing(), 123) + }, 'post-reset usage' () { td.reset() diff --git a/test/unit/imitate/overwrite-children/copy-props.test.js b/test/unit/imitate/overwrite-children/copy-props.test.js index 470e98db..8c043e02 100644 --- a/test/unit/imitate/overwrite-children/copy-props.test.js +++ b/test/unit/imitate/overwrite-children/copy-props.test.js @@ -107,6 +107,87 @@ module.exports = { b: 'b: 12', c: 'c: 13' }) + }, + 'copies the value a getter returns, calling it on the original': () => { + const original = { k: 'kay' } + const target = {} + let receiver + + subject(target, { + a: getterDescriptorFor(function () { + receiver = this + return this.k + }) + }, undefined, original) + + assert.equal(target.a, 'kay') + assert.strictEqual(receiver, original) + }, + 'passes the value a getter returns through the visitor': () => { + const target = {} + + subject(target, { + a: getterDescriptorFor(() => 1) + }, (name, value) => `${name}: ${value + 10}`, {}) + + assert.equal(target.a, 'a: 11') + }, + 'copies undefined for a getter that throws': () => { + const target = {} + + subject(target, { + a: getterDescriptorFor(() => { throw new Error('nope') }) + }, undefined, {}) + + assert.ok('a' in target) + assert.strictEqual(target.a, undefined) + }, + 'copies undefined when the visitor throws on a getter\'s value': () => { + const target = {} + + subject(target, { + a: getterDescriptorFor(() => 1) + }, () => { throw new Error('nope') }, {}) + + assert.ok('a' in target) + assert.strictEqual(target.a, undefined) + }, + 'copies undefined for a setter-only accessor': () => { + const target = {} + + subject(target, { + a: { set (v) {}, configurable: true, enumerable: true } + }, undefined, {}) + + assert.ok('a' in target) + assert.strictEqual(target.a, undefined) + }, + 'does not call getters when the original is a prototype': () => { + class Foo {} + const target = {} + let called = false + + subject(target, { + a: getterDescriptorFor(() => { called = true }) + }, undefined, Foo.prototype) + + assert.ok('a' in target) + assert.strictEqual(target.a, undefined) + assert.strictEqual(called, false) + }, + 'stops expanding getters 3 levels deep': () => { + const makeLink = () => ({ get next () { return makeLink() } }) + const visitor = (name, value) => { + const nested = {} + subject(nested, Object.getOwnPropertyDescriptors(value), visitor, value) + return nested + } + const target = {} + + subject(target, { next: getterDescriptorFor(() => makeLink()) }, visitor, {}) + + assert.ok(target.next.next.next) + assert.strictEqual(target.next.next.next.next, undefined) } } @@ -117,3 +198,9 @@ const basicPropDescriptorFor = (val, except = {}) => configurable: true, enumerable: true }, except) + +const getterDescriptorFor = (get) => ({ + get, + configurable: true, + enumerable: true +}) diff --git a/test/unit/imitate/overwrite-children/index.test.js b/test/unit/imitate/overwrite-children/index.test.js index 3e44675f..56c2d9db 100644 --- a/test/unit/imitate/overwrite-children/index.test.js +++ b/test/unit/imitate/overwrite-children/index.test.js @@ -37,7 +37,7 @@ module.exports = { subject('og', 'target', overwriteChildren) - td.verify(copyProps('target', 'props', propCaptor.capture())) + td.verify(copyProps('target', 'props', propCaptor.capture(), 'og')) // Phase two: test the prop overwriter directly td.when(overwriteChildren('og val', '.aName')).thenReturn('new val')