Merge pull request #550 from testdouble/fix-526-getters

Fix getters being imitated as `undefined`, and `td.replace` on getter properties

Author: rosston

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
 

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])
     }
   }
 }

examples/babel/lib/operations.js

@@ -0,0 +1,2 @@
+// Babel compiles re-exports like this one to getters
+export { square } from './squarer'

examples/babel/lib/squarer.js

@@ -0,0 +1,3 @@
+export function square (n) {
+  throw 'not implemented!'
+}

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!')
+    })
   })
 })

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
 }

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)
   }
 }

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)

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)
   }
 }

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 }
+})