From 2a33596a10b395e53193c6a6da11c52174617acb Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 17:56:29 +0200 Subject: [PATCH 01/14] fix: do not override properties with setters when diffing objects --- CHANGELOG.md | 2 ++ .../__tests__/__snapshots__/matchers.test.js.snap | 12 ++++++++++++ packages/expect/src/__tests__/matchers.test.js | 11 +++++++++++ packages/jest-matcher-utils/src/Replaceable.ts | 9 +++++++-- 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cf5560c5db9..3e2c537fc7c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ### Features ### Fixes + +- `[jest-matcher-utils]` Do not override properties with setters when diffing objects - `[@jest/watcher]` Correct return type of `shouldRunTestSuite` for `JestHookEmitter` ([#9753](https://github.com/facebook/jest/pull/9753)) ### Chore & Maintenance diff --git a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap index 4da9c13f4d09..0924630e4ee8 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -2082,6 +2082,18 @@ Expected: "abc" Received: {"0": "a", "1": "b", "2": "c"} `; +exports[`.toEqual() {pass: false} expect({"a": "foo"}).toEqual({"a": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "a": "bar", ++ "a": "foo", + } +`; + exports[`.toEqual() {pass: false} expect({"a": 1, "b": 2}).toEqual(ObjectContaining {"a": 2}) 1`] = ` expect(received).toEqual(expected) // deep equality diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index 7bb6676b2b24..bca4c89c5bc5 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -435,6 +435,17 @@ describe('.toEqual()', () => { [{a: 1}, {a: 2}], [{a: 5}, {b: 6}], [Object.freeze({foo: {bar: 1}}), {foo: {}}], + [ + { + get a() { + return 'foo'; + }, + set a(value) { + throw new Error('noo'); + }, + }, + {a: 'bar'}, + ], ['banana', 'apple'], ['1\u{00A0}234,57\u{00A0}$', '1 234,57 $'], // issues/6881 [ diff --git a/packages/jest-matcher-utils/src/Replaceable.ts b/packages/jest-matcher-utils/src/Replaceable.ts index 6d1e8a2776c1..b09a32d1b08f 100644 --- a/packages/jest-matcher-utils/src/Replaceable.ts +++ b/packages/jest-matcher-utils/src/Replaceable.ts @@ -37,7 +37,7 @@ export default class Replaceable { }); Object.getOwnPropertySymbols(this.object).forEach(key => { const descriptor = Object.getOwnPropertyDescriptor(this.object, key); - if ((descriptor as PropertyDescriptor).enumerable) { + if (descriptor?.enumerable) { cb(this.object[key], key, this.object); } }); @@ -57,7 +57,12 @@ export default class Replaceable { if (this.type === 'map') { this.object.set(key, value); } else { - this.object[key] = value; + const descriptor = Object.getOwnPropertyDescriptor(this.object, key); + + // do not try to assign to anything that has a setter + if (descriptor?.set === undefined) { + this.object[key] = value; + } } } } From 4dcb1977aba624f65c3e4970e5fe7db5241f8f89 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 18:00:11 +0200 Subject: [PATCH 02/14] link to PR --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e2c537fc7c0..7321116f689d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixes -- `[jest-matcher-utils]` Do not override properties with setters when diffing objects +- `[jest-matcher-utils]` Do not override properties with setters when diffing objects ([#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 From 395abcd9673c1051a3808584ffa051f4e1b30a00 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 18:29:09 +0200 Subject: [PATCH 03/14] remove setters rather than ignoring properties that have them --- packages/jest-matcher-utils/src/Replaceable.ts | 7 +------ .../jest-matcher-utils/src/deepCyclicCopyReplaceable.ts | 4 +++- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/packages/jest-matcher-utils/src/Replaceable.ts b/packages/jest-matcher-utils/src/Replaceable.ts index b09a32d1b08f..fc8c7343c647 100644 --- a/packages/jest-matcher-utils/src/Replaceable.ts +++ b/packages/jest-matcher-utils/src/Replaceable.ts @@ -57,12 +57,7 @@ export default class Replaceable { if (this.type === 'map') { this.object.set(key, value); } else { - const descriptor = Object.getOwnPropertyDescriptor(this.object, key); - - // do not try to assign to anything that has a setter - if (descriptor?.set === undefined) { - this.object[key] = value; - } + this.object[key] = value; } } } diff --git a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index 59f5e160b600..9b29489bdd6d 100644 --- a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -59,7 +59,9 @@ function deepCyclicCopyObject(object: T, cycles: WeakMap): T { descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles); } - if (!('set' in descriptor)) { + if ('set' in descriptor) { + descriptor.set = undefined; + } else { descriptor.writable = true; } From 3503d7712ca9ad17714706c202d53aa2bcf88715 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 19:00:37 +0200 Subject: [PATCH 04/14] only set writable if not getter and no setter --- .../__snapshots__/matchers.test.js.snap | 36 ++++++++++++------- .../expect/src/__tests__/matchers.test.js | 14 ++++++-- .../src/deepCyclicCopyReplaceable.ts | 2 +- 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap index 0924630e4ee8..7fb6a23bccc7 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -2082,18 +2082,6 @@ Expected: "abc" Received: {"0": "a", "1": "b", "2": "c"} `; -exports[`.toEqual() {pass: false} expect({"a": "foo"}).toEqual({"a": "bar"}) 1`] = ` -expect(received).toEqual(expected) // deep equality - -- Expected - 1 -+ Received + 1 - - Object { -- "a": "bar", -+ "a": "foo", - } -`; - exports[`.toEqual() {pass: false} expect({"a": 1, "b": 2}).toEqual(ObjectContaining {"a": 2}) 1`] = ` expect(received).toEqual(expected) // deep equality @@ -2125,6 +2113,18 @@ exports[`.toEqual() {pass: false} expect({"a": 5}).toEqual({"b": 6}) 1`] = ` } `; +exports[`.toEqual() {pass: false} expect({"foo": "foo"}).toEqual({"foo": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "foo": "bar", ++ "foo": "foo", + } +`; + exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {}}) 1`] = ` expect(received).toEqual(expected) // deep equality @@ -2139,6 +2139,18 @@ exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {} } `; +exports[`.toEqual() {pass: false} expect({"foobar": "foo"}).toEqual({"foobar": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "foobar": "bar", ++ "foobar": "foo", + } +`; + exports[`.toEqual() {pass: false} expect({"nodeName": "div", "nodeType": 1}).toEqual({"nodeName": "p", "nodeType": 1}) 1`] = ` expect(received).toEqual(expected) // deep equality diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index bca4c89c5bc5..2ac227cf5e73 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -437,14 +437,22 @@ describe('.toEqual()', () => { [Object.freeze({foo: {bar: 1}}), {foo: {}}], [ { - get a() { + get foo() { return 'foo'; }, - set a(value) { + set foo(value) { throw new Error('noo'); }, }, - {a: 'bar'}, + {foo: 'bar'}, + ], + [ + Object.freeze({ + get foobar() { + return 'foo'; + }, + }), + {foobar: 'bar'}, ], ['banana', 'apple'], ['1\u{00A0}234,57\u{00A0}$', '1 234,57 $'], // issues/6881 diff --git a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index 9b29489bdd6d..e0b50c31e468 100644 --- a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -61,7 +61,7 @@ function deepCyclicCopyObject(object: T, cycles: WeakMap): T { if ('set' in descriptor) { descriptor.set = undefined; - } else { + } else if (!('get' in descriptor)) { descriptor.writable = true; } From b14a485b33da7d2e14b2cc1d0457a4b83b94084b Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 19:08:59 +0200 Subject: [PATCH 05/14] add some frozen and not example with getters and setters --- .../__snapshots__/matchers.test.js.snap | 72 +++++++++++++++---- .../expect/src/__tests__/matchers.test.js | 47 ++++++++++-- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap index 7fb6a23bccc7..1d54a24fa5a3 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -2113,41 +2113,77 @@ exports[`.toEqual() {pass: false} expect({"a": 5}).toEqual({"b": 6}) 1`] = ` } `; -exports[`.toEqual() {pass: false} expect({"foo": "foo"}).toEqual({"foo": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {}}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 3 + + Object { +- "foo": Object {}, ++ "foo": Object { ++ "bar": 1, ++ }, + } +`; + +exports[`.toEqual() {pass: false} expect({"frozenGetter": "foo"}).toEqual({"frozenGetter": "bar"}) 1`] = ` expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 1 Object { -- "foo": "bar", -+ "foo": "foo", +- "frozenGetter": "bar", ++ "frozenGetter": "foo", } `; -exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {}}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"frozenGetterAndSetter": "foo"}).toEqual({"frozenGetterAndSetter": "bar"}) 1`] = ` expect(received).toEqual(expected) // deep equality - Expected - 1 -+ Received + 3 ++ Received + 1 Object { -- "foo": Object {}, -+ "foo": Object { -+ "bar": 1, -+ }, +- "frozenGetterAndSetter": "bar", ++ "frozenGetterAndSetter": "foo", + } +`; + +exports[`.toEqual() {pass: false} expect({"frozenSetter": undefined}).toEqual({"frozenSetter": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "frozenSetter": "bar", ++ "frozenSetter": undefined, + } +`; + +exports[`.toEqual() {pass: false} expect({"getter": "foo"}).toEqual({"getter": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "getter": "bar", ++ "getter": "foo", } `; -exports[`.toEqual() {pass: false} expect({"foobar": "foo"}).toEqual({"foobar": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"getterAndSetter": "foo"}).toEqual({"getterAndSetter": "bar"}) 1`] = ` expect(received).toEqual(expected) // deep equality - Expected - 1 + Received + 1 Object { -- "foobar": "bar", -+ "foobar": "foo", +- "getterAndSetter": "bar", ++ "getterAndSetter": "foo", } `; @@ -2164,6 +2200,18 @@ exports[`.toEqual() {pass: false} expect({"nodeName": "div", "nodeType": 1}).toE } `; +exports[`.toEqual() {pass: false} expect({"setter": undefined}).toEqual({"setter": "bar"}) 1`] = ` +expect(received).toEqual(expected) // deep equality + +- Expected - 1 ++ Received + 1 + + Object { +- "setter": "bar", ++ "setter": undefined, + } +`; + exports[`.toEqual() {pass: false} expect({"target": {"nodeType": 1, "value": "a"}}).toEqual({"target": {"nodeType": 1, "value": "b"}}) 1`] = ` expect(received).toEqual(expected) // deep equality diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index 2ac227cf5e73..e981d0464936 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -437,22 +437,59 @@ describe('.toEqual()', () => { [Object.freeze({foo: {bar: 1}}), {foo: {}}], [ { - get foo() { + get getterAndSetter() { return 'foo'; }, - set foo(value) { + set getterAndSetter(value) { throw new Error('noo'); }, }, - {foo: 'bar'}, + {getterAndSetter: 'bar'}, ], [ Object.freeze({ - get foobar() { + get frozenGetterAndSetter() { return 'foo'; }, + set frozenGetterAndSetter(value) { + throw new Error('noo'); + }, + }), + {frozenGetterAndSetter: 'bar'}, + ], + [ + { + get getter() { + return 'foo'; + }, + }, + {getter: 'bar'}, + ], + [ + Object.freeze({ + get frozenGetter() { + return 'foo'; + }, + }), + {frozenGetter: 'bar'}, + ], + [ + { + // eslint-disable-next-line accessor-pairs + set setter(value) { + throw new Error('noo'); + }, + }, + {setter: 'bar'}, + ], + [ + Object.freeze({ + // eslint-disable-next-line accessor-pairs + set frozenSetter(value) { + throw new Error('noo'); + }, }), - {foobar: 'bar'}, + {frozenSetter: 'bar'}, ], ['banana', 'apple'], ['1\u{00A0}234,57\u{00A0}$', '1 234,57 $'], // issues/6881 From cf568462b6f9c249bf6c33d863ee94a78f1b6f87 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 19:30:25 +0200 Subject: [PATCH 06/14] use hasOwnProperty --- packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index e0b50c31e468..50e69979c4a7 100644 --- a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -55,7 +55,7 @@ function deepCyclicCopyObject(object: T, cycles: WeakMap): T { Object.keys(descriptors).forEach(key => { const descriptor = descriptors[key]; - if (typeof descriptor.value !== 'undefined') { + if (descriptor.hasOwnProperty('value')) { descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles); } From 8f5824cf4fd8c8cf9d978cb0406d58075a55bf41 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 19:43:23 +0200 Subject: [PATCH 07/14] just create a new descriptor --- .../src/deepCyclicCopyReplaceable.ts | 25 +++++++++---------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index 50e69979c4a7..ad98131dd9d3 100644 --- a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -49,23 +49,22 @@ export default function deepCyclicCopyReplaceable( function deepCyclicCopyObject(object: T, cycles: WeakMap): 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 (descriptor.hasOwnProperty('value')) { - descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles); - } - - if ('set' in descriptor) { - descriptor.set = undefined; - } else if (!('get' in descriptor)) { - descriptor.writable = true; - } - - descriptor.configurable = true; + descriptors[key] = { + configurable: true, + enumerable: true, + value: deepCyclicCopyReplaceable( + (object as Record)[key], + cycles, + ), + writable: true, + }; }); return Object.defineProperties(newObject, descriptors); From 3aed0777e8e023467d233a85ca17e8a9ef071b6a Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 19:46:04 +0200 Subject: [PATCH 08/14] simplify replaceable --- packages/jest-matcher-utils/src/Replaceable.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/jest-matcher-utils/src/Replaceable.ts b/packages/jest-matcher-utils/src/Replaceable.ts index fc8c7343c647..8f518fef1494 100644 --- a/packages/jest-matcher-utils/src/Replaceable.ts +++ b/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?.enumerable) { - cb(this.object[key], key, this.object); - } + cb(this.object[key], key, this.object); }); } else { this.object.forEach(cb); From 9ff198fe089dd6bd717c7fada144c299750a3760 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 20:03:46 +0200 Subject: [PATCH 09/14] write tests that actually fail on master --- .../__snapshots__/matchers.test.js.snap | 56 +++++++++++-------- .../expect/src/__tests__/matchers.test.js | 20 +++---- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap index 1d54a24fa5a3..bc1abc19adf6 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -2127,63 +2127,73 @@ exports[`.toEqual() {pass: false} expect({"foo": {"bar": 1}}).toEqual({"foo": {} } `; -exports[`.toEqual() {pass: false} expect({"frozenGetter": "foo"}).toEqual({"frozenGetter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"frozenGetter": {}}).toEqual({"frozenGetter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "frozenGetter": "bar", -+ "frozenGetter": "foo", +- "frozenGetter": Object { +- "foo": "bar", +- }, ++ "frozenGetter": Object {}, } `; -exports[`.toEqual() {pass: false} expect({"frozenGetterAndSetter": "foo"}).toEqual({"frozenGetterAndSetter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"frozenGetterAndSetter": {}}).toEqual({"frozenGetterAndSetter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "frozenGetterAndSetter": "bar", -+ "frozenGetterAndSetter": "foo", +- "frozenGetterAndSetter": Object { +- "foo": "bar", +- }, ++ "frozenGetterAndSetter": Object {}, } `; -exports[`.toEqual() {pass: false} expect({"frozenSetter": undefined}).toEqual({"frozenSetter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"frozenSetter": undefined}).toEqual({"frozenSetter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "frozenSetter": "bar", +- "frozenSetter": Object { +- "foo": "bar", +- }, + "frozenSetter": undefined, } `; -exports[`.toEqual() {pass: false} expect({"getter": "foo"}).toEqual({"getter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"getter": {}}).toEqual({"getter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "getter": "bar", -+ "getter": "foo", +- "getter": Object { +- "foo": "bar", +- }, ++ "getter": Object {}, } `; -exports[`.toEqual() {pass: false} expect({"getterAndSetter": "foo"}).toEqual({"getterAndSetter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"getterAndSetter": {}}).toEqual({"getterAndSetter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "getterAndSetter": "bar", -+ "getterAndSetter": "foo", +- "getterAndSetter": Object { +- "foo": "bar", +- }, ++ "getterAndSetter": Object {}, } `; @@ -2200,14 +2210,16 @@ exports[`.toEqual() {pass: false} expect({"nodeName": "div", "nodeType": 1}).toE } `; -exports[`.toEqual() {pass: false} expect({"setter": undefined}).toEqual({"setter": "bar"}) 1`] = ` +exports[`.toEqual() {pass: false} expect({"setter": undefined}).toEqual({"setter": {"foo": "bar"}}) 1`] = ` expect(received).toEqual(expected) // deep equality -- Expected - 1 +- Expected - 3 + Received + 1 Object { -- "setter": "bar", +- "setter": Object { +- "foo": "bar", +- }, + "setter": undefined, } `; diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index e981d0464936..8d1ec4fbea8c 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -438,40 +438,40 @@ describe('.toEqual()', () => { [ { get getterAndSetter() { - return 'foo'; + return {}; }, set getterAndSetter(value) { throw new Error('noo'); }, }, - {getterAndSetter: 'bar'}, + {getterAndSetter: {foo: 'bar'}}, ], [ Object.freeze({ get frozenGetterAndSetter() { - return 'foo'; + return {}; }, set frozenGetterAndSetter(value) { throw new Error('noo'); }, }), - {frozenGetterAndSetter: 'bar'}, + {frozenGetterAndSetter: {foo: 'bar'}}, ], [ { get getter() { - return 'foo'; + return {}; }, }, - {getter: 'bar'}, + {getter: {foo: 'bar'}}, ], [ Object.freeze({ get frozenGetter() { - return 'foo'; + return {}; }, }), - {frozenGetter: 'bar'}, + {frozenGetter: {foo: 'bar'}}, ], [ { @@ -480,7 +480,7 @@ describe('.toEqual()', () => { throw new Error('noo'); }, }, - {setter: 'bar'}, + {setter: {foo: 'bar'}}, ], [ Object.freeze({ @@ -489,7 +489,7 @@ describe('.toEqual()', () => { throw new Error('noo'); }, }), - {frozenSetter: 'bar'}, + {frozenSetter: {foo: 'bar'}}, ], ['banana', 'apple'], ['1\u{00A0}234,57\u{00A0}$', '1 234,57 $'], // issues/6881 From 5f72f4ea57b652e9775a8313569737b01318f487 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 20:11:30 +0200 Subject: [PATCH 10/14] remove faulty test and re-add enumerable check --- packages/jest-matcher-utils/src/Replaceable.ts | 5 ++++- .../src/__tests__/Replaceable.test.ts | 4 ++-- .../__tests__/deepCyclicCopyReplaceable.test.ts | 14 -------------- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/packages/jest-matcher-utils/src/Replaceable.ts b/packages/jest-matcher-utils/src/Replaceable.ts index 8f518fef1494..fc8c7343c647 100644 --- a/packages/jest-matcher-utils/src/Replaceable.ts +++ b/packages/jest-matcher-utils/src/Replaceable.ts @@ -36,7 +36,10 @@ export default class Replaceable { cb(value, key, this.object); }); Object.getOwnPropertySymbols(this.object).forEach(key => { - cb(this.object[key], key, this.object); + const descriptor = Object.getOwnPropertyDescriptor(this.object, key); + if (descriptor?.enumerable) { + cb(this.object[key], key, this.object); + } }); } else { this.object.forEach(cb); diff --git a/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts b/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts index aa724bfe968e..1ba92718b3b9 100644 --- a/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts +++ b/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts @@ -104,7 +104,7 @@ 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]); @@ -117,7 +117,7 @@ describe('Replaceable', () => { const replaceable = new Replaceable(object); const cb = jest.fn(); replaceable.forEach(cb); - expect(cb.mock.calls.length).toBe(2); + expect(cb).toHaveBeenCalledTimes(2); expect(cb.mock.calls[0]).toEqual([1, 'a', object]); expect(cb.mock.calls[1]).toEqual([2, 'b', object]); }); diff --git a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts index 3063db1f76f7..c142b5022eee 100644 --- a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts +++ b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts @@ -20,20 +20,6 @@ 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(); - const obj = { - // @ts-ignore - get foo() { - fn(); - }, - }; - const copy = deepCyclicCopyReplaceable(obj); - - expect(Object.getOwnPropertyDescriptor(copy, 'foo')).toBeDefined(); - expect(fn).not.toBeCalled(); -}); - test('copies symbols', () => { const symbol = Symbol('foo'); const obj = {[symbol]: 42}; From 30495cadce7a1576e220e2a271c07e6c3094a3d2 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 20:45:21 +0200 Subject: [PATCH 11/14] move enumarable check --- .../jest-matcher-utils/src/Replaceable.ts | 5 +--- .../src/__tests__/Replaceable.test.ts | 14 ++--------- .../src/deepCyclicCopyReplaceable.ts | 24 ++++++++++++------- 3 files changed, 18 insertions(+), 25 deletions(-) diff --git a/packages/jest-matcher-utils/src/Replaceable.ts b/packages/jest-matcher-utils/src/Replaceable.ts index fc8c7343c647..8f518fef1494 100644 --- a/packages/jest-matcher-utils/src/Replaceable.ts +++ b/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?.enumerable) { - cb(this.object[key], key, this.object); - } + cb(this.object[key], key, this.object); }); } else { this.object.forEach(cb); diff --git a/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts b/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts index 1ba92718b3b9..890f1ba38b96 100644 --- a/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts +++ b/packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts @@ -110,22 +110,11 @@ describe('Replaceable', () => { 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).toHaveBeenCalledTimes(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]); }); diff --git a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts index ad98131dd9d3..6849735a7f2c 100644 --- a/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts +++ b/packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts @@ -56,15 +56,21 @@ function deepCyclicCopyObject(object: T, cycles: WeakMap): T { cycles.set(object, newObject); Object.keys(descriptors).forEach(key => { - descriptors[key] = { - configurable: true, - enumerable: true, - value: deepCyclicCopyReplaceable( - (object as Record)[key], - cycles, - ), - writable: true, - }; + 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)[key], + cycles, + ), + writable: true, + }; + } else { + delete descriptors[key]; + } }); return Object.defineProperties(newObject, descriptors); From fe417879a654bf1733409f51d95633bcdc126954 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 20:45:44 +0200 Subject: [PATCH 12/14] add test for getter -> value change --- .../deepCyclicCopyReplaceable.test.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts index c142b5022eee..6debdcfc2b10 100644 --- a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts +++ b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts @@ -20,6 +20,29 @@ test('returns the same value for primitive or function values', () => { expect(deepCyclicCopyReplaceable(fn)).toBe(fn); }); +test('convert accessor descriptor into value descriptor', () => { + const obj = { + set foo(_) {}, + get foo() { + 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')).toEqual({ + configurable: true, + enumerable: true, + value: 'bar', + writable: true, + }); +}); + test('copies symbols', () => { const symbol = Symbol('foo'); const obj = {[symbol]: 42}; From 1aea89b954c2294b349255d873956d32e3c86084 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Thu, 2 Apr 2020 20:49:28 +0200 Subject: [PATCH 13/14] test enumerable skip --- .../src/__tests__/deepCyclicCopyReplaceable.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts index 6debdcfc2b10..63af3a7dce3d 100644 --- a/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts +++ b/packages/jest-matcher-utils/src/__tests__/deepCyclicCopyReplaceable.test.ts @@ -43,6 +43,15 @@ test('convert accessor descriptor into value descriptor', () => { }); }); +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', () => { const symbol = Symbol('foo'); const obj = {[symbol]: 42}; From c0edc891fdf1364c410a9dcb48f274d9da3b288e Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Fri, 3 Apr 2020 08:47:02 +0200 Subject: [PATCH 14/14] update changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7321116f689d..fa8546a16c5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ ### Fixes -- `[jest-matcher-utils]` Do not override properties with setters when diffing objects ([#9757](https://github.com/facebook/jest/pull/9757)) +- `[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