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]` 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
Expand Down
12 changes: 12 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -2082,6 +2082,18 @@ Expected: <g>"abc"</>
Received: <r>{"0": "a", "1": "b", "2": "c"}</>
`;

exports[`.toEqual() {pass: false} expect({"a": "foo"}).toEqual({"a": "bar"}) 1`] = `
<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>

<g>- Expected - 1</>
<r>+ Received + 1</>

<d> Object {</>
<g>- "a": "bar",</>
<r>+ "a": "foo",</>
<d> }</>
`;

exports[`.toEqual() {pass: false} expect({"a": 1, "b": 2}).toEqual(ObjectContaining {"a": 2}) 1`] = `
<d>expect(</><r>received</><d>).</>toEqual<d>(</><g>expected</><d>) // deep equality</>

Expand Down
11 changes: 11 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,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
[
Expand Down
2 changes: 1 addition & 1 deletion packages/jest-matcher-utils/src/Replaceable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {

This comment was marked as outdated.

if (descriptor?.enumerable) {
cb(this.object[key], key, this.object);
}
});
Expand Down
4 changes: 3 additions & 1 deletion packages/jest-matcher-utils/src/deepCyclicCopyReplaceable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ function deepCyclicCopyObject<T>(object: T, cycles: WeakMap<any, any>): T {
descriptor.value = deepCyclicCopyReplaceable(descriptor.value, cycles);
}

if (!('set' in descriptor)) {
if ('set' in descriptor) {
gaearon marked this conversation as resolved.
Show resolved Hide resolved
descriptor.set = undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if setting to undefined or deleting it makes the most sense

} else {
descriptor.writable = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be outside a condition? You could have a frozen object with a setter on it, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot set writable if there's a getter or a setter. So thanks for the question, we still failed if there was a getter but no setter 🙂 fixed and added a test

Object.defineProperty({}, 'foo', {set(_){}, writable: true}) // Invalid property descriptor. Cannot both specify accessors and a value or writable attribute
Object.defineProperty({}, 'foo', {get(){}, writable: true}) // Invalid property descriptor. Cannot both specify accessors and a value or writable attribute

Copy link
Member Author

Choose a reason for hiding this comment

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

Or did you have another example in mind?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some more tests. With both getter and setter, only getter and only setter, both wrapped in Object.freeze and not.

}

Expand Down