fix: drop getters and setters when diffing objects for error (#9757)

Author: SimenB

CHANGELOG.md

@@ -3,6 +3,8 @@
 ### Features
 
 ### Fixes
+
+- `[jest-matcher-utils]` Replace accessors with values to avoid calling setters in object descriptors when computing diffs for error reporting ([#9757](https://github.com/facebook/jest/pull/9757))
 - `[@jest/watcher]` Correct return type of `shouldRunTestSuite` for `JestHookEmitter` ([#9753](https://github.com/facebook/jest/pull/9753))
 
 ### Chore & Maintenance

packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap

@@ -2127,6 +2127,76 @@ exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {}
 <d>  }</>
 `;
 
+exports[`.toEqual() {pass: false} expect({"frozenGetter": {}}).toEqual({"frozenGetter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "frozenGetter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "frozenGetter": Object {},</>
+<d>  }</>
+`;
+
+exports[`.toEqual() {pass: false} expect({"frozenGetterAndSetter": {}}).toEqual({"frozenGetterAndSetter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "frozenGetterAndSetter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "frozenGetterAndSetter": Object {},</>
+<d>  }</>
+`;
+
+exports[`.toEqual() {pass: false} expect({"frozenSetter": undefined}).toEqual({"frozenSetter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "frozenSetter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "frozenSetter": undefined,</>
+<d>  }</>
+`;
+
+exports[`.toEqual() {pass: false} expect({"getter": {}}).toEqual({"getter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "getter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "getter": Object {},</>
+<d>  }</>
+`;
+
+exports[`.toEqual() {pass: false} expect({"getterAndSetter": {}}).toEqual({"getterAndSetter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "getterAndSetter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "getterAndSetter": Object {},</>
+<d>  }</>
+`;
+
 exports[`.toEqual() {pass: false} expect({"nodeName": "div", "nodeType": 1}).toEqual({"nodeName": "p", "nodeType": 1}) 1`] = `
 <d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
 
@@ -2140,6 +2210,20 @@ exports[`.toEqual() {pass: false} expect({"nodeName": "div", "nodeType": 1}).toE
 <d>  }</>
 `;
 
+exports[`.toEqual() {pass: false} expect({"setter": undefined}).toEqual({"setter": {"foo": "bar"}}) 1`] = `
+<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
+
+<g>- Expected  - 3</>
+<r>+ Received  + 1</>
+
+<d>  Object {</>
+<g>-   "setter": Object {</>
+<g>-     "foo": "bar",</>
+<g>-   },</>
+<r>+   "setter": undefined,</>
+<d>  }</>
+`;
+
 exports[`.toEqual() {pass: false} expect({"target": {"nodeType": 1, "value": "a"}}).toEqual({"target": {"nodeType": 1, "value": "b"}}) 1`] = `
 <d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>
 

packages/expect/src/__tests__/matchers.test.js

@@ -435,6 +435,62 @@ describe('.toEqual()', () => {
     [{a: 1}, {a: 2}],
     [{a: 5}, {b: 6}],
     [Object.freeze({foo: {bar: 1}}), {foo: {}}],
+    [
+      {
+        get getterAndSetter() {
+          return {};
+        },
+        set getterAndSetter(value) {
+          throw new Error('noo');
+        },
+      },
+      {getterAndSetter: {foo: 'bar'}},
+    ],
+    [
+      Object.freeze({
+        get frozenGetterAndSetter() {
+          return {};
+        },
+        set frozenGetterAndSetter(value) {
+          throw new Error('noo');
+        },
+      }),
+      {frozenGetterAndSetter: {foo: 'bar'}},
+    ],
+    [
+      {
+        get getter() {
+          return {};
+        },
+      },
+      {getter: {foo: 'bar'}},
+    ],
+    [
+      Object.freeze({
+        get frozenGetter() {
+          return {};
+        },
+      }),
+      {frozenGetter: {foo: 'bar'}},
+    ],
+    [
+      {
+        // eslint-disable-next-line accessor-pairs
+        set setter(value) {
+          throw new Error('noo');
+        },
+      },
+      {setter: {foo: 'bar'}},
+    ],
+    [
+      Object.freeze({
+        // eslint-disable-next-line accessor-pairs
+        set frozenSetter(value) {
+          throw new Error('noo');
+        },
+      }),
+      {frozenSetter: {foo: 'bar'}},
+    ],
     ['banana', 'apple'],
     ['1\u{00A0}234,57\u{00A0}$', '1 234,57 $'], // issues/6881
     [

packages/jest-matcher-utils/src/Replaceable.ts

@@ -36,10 +36,7 @@ export default class Replaceable {
         cb(value, key, this.object);
       });
       Object.getOwnPropertySymbols(this.object).forEach(key => {
-        const descriptor = Object.getOwnPropertyDescriptor(this.object, key);
-        if ((descriptor as PropertyDescriptor).enumerable) {
-          cb(this.object[key], key, this.object);
-        }
+        cb(this.object[key], key, this.object);
       });
     } else {
       this.object.forEach(cb);

packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts

@@ -104,28 +104,17 @@ describe('Replaceable', () => {
       const replaceable = new Replaceable(object);
       const cb = jest.fn();
       replaceable.forEach(cb);
-      expect(cb.mock.calls.length).toBe(3);
+      expect(cb).toHaveBeenCalledTimes(3);
       expect(cb.mock.calls[0]).toEqual([1, 'a', object]);
       expect(cb.mock.calls[1]).toEqual([2, 'b', object]);
       expect(cb.mock.calls[2]).toEqual([3, symbolKey, object]);
     });
 
-    test('object forEach do not iterate none enumerable symbol key', () => {
-      const symbolKey = Symbol('jest');
-      const object = {a: 1, b: 2};
-      Object.defineProperty(object, symbolKey, {enumerable: false});
-      const replaceable = new Replaceable(object);
-      const cb = jest.fn();
-      replaceable.forEach(cb);
-      expect(cb.mock.calls.length).toBe(2);
-      expect(cb.mock.calls[0]).toEqual([1, 'a', object]);
-      expect(cb.mock.calls[1]).toEqual([2, 'b', object]);
-    });
-
     test('array forEach', () => {
       const replaceable = new Replaceable([1, 2, 3]);
       const cb = jest.fn();
       replaceable.forEach(cb);
+      expect(cb).toHaveBeenCalledTimes(3);
       expect(cb.mock.calls[0]).toEqual([1, 0, [1, 2, 3]]);
       expect(cb.mock.calls[1]).toEqual([2, 1, [1, 2, 3]]);
       expect(cb.mock.calls[2]).toEqual([3, 2, [1, 2, 3]]);
@@ -139,6 +128,7 @@ describe('Replaceable', () => {
       const replaceable = new Replaceable(map);
       const cb = jest.fn();
       replaceable.forEach(cb);
+      expect(cb).toHaveBeenCalledTimes(2);
       expect(cb.mock.calls[0]).toEqual([1, 'a', map]);
       expect(cb.mock.calls[1]).toEqual([2, 'b', map]);
     });

packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts

@@ -20,18 +20,36 @@ test('returns the same value for primitive or function values', () => {
   expect(deepCyclicCopyReplaceable(fn)).toBe(fn);
 });
 
-test('does not execute getters/setters, but copies them', () => {
-  const fn = jest.fn();
+test('convert accessor descriptor into value descriptor', () => {
   const obj = {
-    // @ts-ignore
+    set foo(_) {},
     get foo() {
-      fn();
+      return 'bar';
     },
   };
+  expect(Object.getOwnPropertyDescriptor(obj, 'foo')).toEqual({
+    configurable: true,
+    enumerable: true,
+    get: expect.any(Function),
+    set: expect.any(Function),
+  });
   const copy = deepCyclicCopyReplaceable(obj);
 
-  expect(Object.getOwnPropertyDescriptor(copy, 'foo')).toBeDefined();
-  expect(fn).not.toBeCalled();
+  expect(Object.getOwnPropertyDescriptor(copy, 'foo')).toEqual({
+    configurable: true,
+    enumerable: true,
+    value: 'bar',
+    writable: true,
+  });
+});
+
+test('skips non-enumerables', () => {
+  const obj = {};
+  Object.defineProperty(obj, 'foo', {enumerable: false, value: 'bar'});
+
+  const copy = deepCyclicCopyReplaceable(obj);
+
+  expect(Object.getOwnPropertyDescriptors(copy)).toEqual({});
 });
 
 test('copies symbols', () => {

packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts

@@ -49,21 +49,28 @@ export default function deepCyclicCopyReplaceable<T>(
 
 function deepCyclicCopyObject<T>(object: T, cycles: WeakMap<any, any>): T {
   const newObject = Object.create(Object.getPrototypeOf(object));
-  const descriptors = Object.getOwnPropertyDescriptors(object);
+  const descriptors: {
+    [x: string]: PropertyDescriptor;
+  } = Object.getOwnPropertyDescriptors(object);
 
   cycles.set(object, newObject);
 
   Object.keys(descriptors).forEach(key => {
-    const descriptor = descriptors[key];
-    if (typeof descriptor.value !== 'undefined') {
-      descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles);
+    if (descriptors[key].enumerable) {
+      descriptors[key] = {
+        configurable: true,
+        enumerable: true,
+        value: deepCyclicCopyReplaceable(
+          // this accesses the value or getter, depending. We just care about the value anyways, and this allows us to not mess with accessors
+          // it has the side effect of invoking the getter here though, rather than copying it over
+          (object as Record<string, unknown>)[key],
+          cycles,
+        ),
+        writable: true,
+      };
+    } else {
+      delete descriptors[key];
     }
-
-    if (!('set' in descriptor)) {
-      descriptor.writable = true;
-    }
-
-    descriptor.configurable = true;
   });
 
   return Object.defineProperties(newObject, descriptors);