Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 14 commits into from
Apr 3, 2020
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
### Features

### Fixes

Copy link
Member Author

@SimenB SimenB Apr 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is failing on master due to this missing newline, snuck it in here

- `[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
Expand Down
84 changes: 84 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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</>

Expand All @@ -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</>

Expand Down
56 changes: 56 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
[
Expand Down
5 changes: 1 addition & 4 deletions packages/jest-matcher-utils/src/Replaceable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

This comment was marked as outdated.

cb(this.object[key], key, this.object);
}
cb(this.object[key], key, this.object);
});
} else {
this.object.forEach(cb);
Expand Down
16 changes: 3 additions & 13 deletions packages/jest-matcher-utils/src/__tests__/Replaceable.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check moved

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]]);
Expand All @@ -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]);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it's not just in the comments - we invoke the getters on purpose now, so this test doesn't make sense anymore

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', () => {
Expand Down
27 changes: 17 additions & 10 deletions packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down