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.

Sign up for GitHub

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

Jump to bottom

Fix handling circular references correctly in objects (closes #8663) #8687

Merged
merged 2 commits into from Jul 28, 2019
Merged

Fix handling circular references correctly in objects (closes #8663) #8687

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c6eb48
fix: handle circular references correctly in objects (closes #8663)
lucasfcosta Jul 13, 2019
1f7bf0a
fix: handling circular references properly in getObjectSubset (#8663)
lucasfcosta Jul 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@
- `[jest-core]` Fix incorrect `passWithNoTests` warning ([#8595](https://github.com/facebook/jest/pull/8595))
- `[jest-snapshots]` Fix test retries that contain snapshots ([#8629](https://github.com/facebook/jest/pull/8629))
- `[jest-mock]` Fix incorrect assignments when restoring mocks in instances where they originally didn't exist ([#8631](https://github.com/facebook/jest/pull/8631))
- `[expect]` Fix stack overflow when matching objects with circular references ([#8687](https://github.com/facebook/jest/pull/8687))

### Chore & Maintenance

Expand Down
98 changes: 98 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
View file
Open in desktop
Expand Up @@ -4092,6 +4092,104 @@ Expected: not <green>Set {2, 1}</>
Received: <red>Set {1, 2}</>"
`;

exports[`toMatchObject() circular references simple circular references {pass: false} expect({"a": "hello", "ref": [Circular]}).toMatchObject({"a": "world", "ref": [Circular]}) 1`] = `"Maximum call stack size exceeded"`;

exports[`toMatchObject() circular references simple circular references {pass: false} expect({"ref": "not a ref"}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

<green>- Expected</>
<red>+ Received</>

<dim> Object {</>
<green>- \\"a\\": \\"hello\\",</>
<green>- \\"ref\\": [Circular],</>
<red>+ \\"ref\\": \\"not a ref\\",</>
<dim> }</>"
`;

exports[`toMatchObject() circular references simple circular references {pass: false} expect({}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

<green>- Expected</>
<red>+ Received</>

<green>- Object {</>
<green>- \\"a\\": \\"hello\\",</>
<green>- \\"ref\\": [Circular],</>
<green>- }</>
<red>+ Object {}</>"
`;

exports[`toMatchObject() circular references simple circular references {pass: true} expect({"a": "hello", "ref": [Circular]}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toMatchObject<dim>(</><green>expected</><dim>)</>

Expected: not <green>{\\"a\\": \\"hello\\", \\"ref\\": [Circular]}</>"
`;

exports[`toMatchObject() circular references simple circular references {pass: true} expect({"a": "hello", "ref": [Circular]}).toMatchObject({}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toMatchObject<dim>(</><green>expected</><dim>)</>

Expected: not <green>{}</>
Received: <red>{\\"a\\": \\"hello\\", \\"ref\\": [Circular]}</>"
`;

exports[`toMatchObject() circular references transitive circular references {pass: false} expect({"a": "world", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = `"Maximum call stack size exceeded"`;
Copy link
Contributor

Choose a reason for hiding this comment

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

@lucasfcosta I have added some line breaks so you can verify if the long-line snapshot is the intended criterion:

exports[
`toMatchObject() circular references simple circular references {pass: false}
expect({"a": "hello", "ref": [Circular]})
.toMatchObject({"a": "world", "ref": [Circular]}) 1`
] =
`"Maximum call stack size exceeded"`;

I think it is from test case: [circularObjA1, circularObjB]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Excellent catch! I have just fixed that. Please see the Appendix below.

Thank you @pedrottimark ✨


exports[`toMatchObject() circular references transitive circular references {pass: false} expect({"nestedObj": {"parentObj": "not the parent ref"}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

<green>- Expected</>
<red>+ Received</>

<dim> Object {</>
<green>- \\"a\\": \\"hello\\",</>
<dim> \\"nestedObj\\": Object {</>
<green>- \\"parentObj\\": [Circular],</>
<red>+ \\"parentObj\\": \\"not the parent ref\\",</>
<dim> },</>
<dim> }</>"
`;

exports[`toMatchObject() circular references transitive circular references {pass: false} expect({}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

<green>- Expected</>
<red>+ Received</>

<green>- Object {</>
<green>- \\"a\\": \\"hello\\",</>
<green>- \\"nestedObj\\": Object {</>
<green>- \\"parentObj\\": [Circular],</>
<green>- },</>
<green>- }</>
<red>+ Object {}</>"
`;

exports[`toMatchObject() circular references transitive circular references {pass: true} expect({"a": "hello", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toMatchObject<dim>(</><green>expected</><dim>)</>

Expected: not <green>{\\"a\\": \\"hello\\", \\"nestedObj\\": {\\"parentObj\\": [Circular]}}</>"
`;

exports[`toMatchObject() circular references transitive circular references {pass: true} expect({"a": "hello", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toMatchObject<dim>(</><green>expected</><dim>)</>

Expected: not <green>{}</>
Received: <red>{\\"a\\": \\"hello\\", \\"nestedObj\\": {\\"parentObj\\": [Circular]}}</>"
`;

exports[`toMatchObject() does not match properties up in the prototype chain 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

<green>- Expected</>
<red>+ Received</>

<dim> Object {</>
<dim> \\"other\\": \\"child\\",</>
<green>- \\"ref\\": [Circular],</>
<dim> }</>"
`;

exports[`toMatchObject() throws expect("44").toMatchObject({}) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toMatchObject<dim>(</><green>expected</><dim>)</>

Expand Down
126 changes: 104 additions & 22 deletions packages/expect/src/__tests__/matchers.test.js
View file
Open in desktop
Expand Up @@ -1537,7 +1537,91 @@ describe('toMatchObject()', () => {
}
}

[
const testNotToMatchSnapshots = tuples => {
tuples.forEach(([n1, n2]) => {
it(`{pass: true} expect(${stringify(n1)}).toMatchObject(${stringify(
n2,
)})`, () => {
jestExpect(n1).toMatchObject(n2);
expect(() =>
jestExpect(n1).not.toMatchObject(n2),
).toThrowErrorMatchingSnapshot();
});
});
};

const testToMatchSnapshots = tuples => {
tuples.forEach(([n1, n2]) => {
it(`{pass: false} expect(${stringify(n1)}).toMatchObject(${stringify(
n2,
)})`, () => {
jestExpect(n1).not.toMatchObject(n2);
expect(() =>
jestExpect(n1).toMatchObject(n2),
).toThrowErrorMatchingSnapshot();
});
});
};

describe('circular references', () => {
describe('simple circular references', () => {
const circularObjA1 = {a: 'hello'};
circularObjA1.ref = circularObjA1;

const circularObjB = {a: 'world'};
circularObjB.ref = circularObjB;

const circularObjA2 = {a: 'hello'};
circularObjA2.ref = circularObjA2;

const primitiveInsteadOfRef = {};
primitiveInsteadOfRef.ref = 'not a ref';

testNotToMatchSnapshots([
[circularObjA1, {}],
[circularObjA2, circularObjA1],
]);

testToMatchSnapshots([
[{}, circularObjA1],
[circularObjA1, circularObjB],
[primitiveInsteadOfRef, circularObjA1],
]);
});

describe('transitive circular references', () => {
const transitiveCircularObjA1 = {a: 'hello'};
transitiveCircularObjA1.nestedObj = {parentObj: transitiveCircularObjA1};

const transitiveCircularObjA2 = {a: 'hello'};
transitiveCircularObjA2.nestedObj = {
parentObj: transitiveCircularObjA2,
};

const transitiveCircularObjB = {a: 'world'};
transitiveCircularObjB.nestedObj = {
parentObj: transitiveCircularObjB,
};

const primitiveInsteadOfRef = {};
primitiveInsteadOfRef.nestedObj = {
parentObj: 'not the parent ref',
};

testNotToMatchSnapshots([
[transitiveCircularObjA1, {}],
[transitiveCircularObjA2, transitiveCircularObjA1],
]);

testToMatchSnapshots([
[{}, transitiveCircularObjA1],
[transitiveCircularObjB, transitiveCircularObjA1],
[primitiveInsteadOfRef, transitiveCircularObjA1],
]);
});
});

testNotToMatchSnapshots([
[{a: 'b', c: 'd'}, {a: 'b'}],
[{a: 'b', c: 'd'}, {a: 'b', c: 'd'}],
[{a: 'b', t: {x: {r: 'r'}, z: 'z'}}, {a: 'b', t: {z: 'z'}}],
Expand All @@ -1560,18 +1644,9 @@ describe('toMatchObject()', () => {
[new Error('bar'), {message: 'bar'}],
[new Foo(), {a: undefined, b: 'b'}],
[Object.assign(Object.create(null), {a: 'b'}), {a: 'b'}],
].forEach(([n1, n2]) => {
it(`{pass: true} expect(${stringify(n1)}).toMatchObject(${stringify(
n2,
)})`, () => {
jestExpect(n1).toMatchObject(n2);
expect(() =>
jestExpect(n1).not.toMatchObject(n2),
).toThrowErrorMatchingSnapshot();
});
});
]);

[
testToMatchSnapshots([
[{a: 'b', c: 'd'}, {e: 'b'}],
[{a: 'b', c: 'd'}, {a: 'b!', c: 'd'}],
[{a: 'a', c: 'd'}, {a: jestExpect.any(Number)}],
Expand All @@ -1597,16 +1672,7 @@ describe('toMatchObject()', () => {
[[1, 2, 3], [1, 2, 2]],
[new Error('foo'), new Error('bar')],
[Object.assign(Object.create(null), {a: 'b'}), {c: 'd'}],
].forEach(([n1, n2]) => {
it(`{pass: false} expect(${stringify(n1)}).toMatchObject(${stringify(
n2,
)})`, () => {
jestExpect(n1).not.toMatchObject(n2);
expect(() =>
jestExpect(n1).toMatchObject(n2),
).toThrowErrorMatchingSnapshot();
});
});
]);

[
[null, {}],
Expand All @@ -1628,4 +1694,20 @@ describe('toMatchObject()', () => {
).toThrowErrorMatchingSnapshot();
});
});

it('does not match properties up in the prototype chain', () => {
const a = {};
a.ref = a;

const b = Object.create(a);
b.other = 'child';

const matcher = {other: 'child'};
matcher.ref = matcher;

jestExpect(b).not.toMatchObject(matcher);
expect(() =>
jestExpect(b).toMatchObject(matcher),
).toThrowErrorMatchingSnapshot();
});
});
54 changes: 54 additions & 0 deletions packages/expect/src/__tests__/utils.test.js
View file
Open in desktop
Expand Up @@ -202,6 +202,60 @@ describe('subsetEquality()', () => {
test('undefined does not return errors', () => {
expect(subsetEquality(undefined, {foo: 'bar'})).not.toBeTruthy();
});

describe('matching subsets with circular references', () => {
test('simple circular references', () => {
const circularObjA1 = {a: 'hello'};
circularObjA1.ref = circularObjA1;

const circularObjA2 = {a: 'hello'};
circularObjA2.ref = circularObjA2;

const circularObjB = {a: 'world'};
circularObjB.ref = circularObjB;

const primitiveInsteadOfRef = {};
primitiveInsteadOfRef.ref = 'not a ref';

expect(subsetEquality(circularObjA1, {})).toBe(true);
expect(subsetEquality({}, circularObjA1)).toBe(false);
expect(subsetEquality(circularObjA2, circularObjA1)).toBe(true);
expect(subsetEquality(circularObjB, circularObjA1)).toBe(false);
expect(subsetEquality(primitiveInsteadOfRef, circularObjA1)).toBe(false);
});

test('transitive circular references', () => {
const transitiveCircularObjA1 = {a: 'hello'};
transitiveCircularObjA1.nestedObj = {parentObj: transitiveCircularObjA1};

const transitiveCircularObjA2 = {a: 'hello'};
transitiveCircularObjA2.nestedObj = {
parentObj: transitiveCircularObjA2,
};

const transitiveCircularObjB = {a: 'world'};
transitiveCircularObjB.nestedObj = {
parentObj: transitiveCircularObjB,
};

const primitiveInsteadOfRef = {};
primitiveInsteadOfRef.nestedObj = {
parentObj: 'not the parent ref',
};

expect(subsetEquality(transitiveCircularObjA1, {})).toBe(true);
expect(subsetEquality({}, transitiveCircularObjA1)).toBe(false);
expect(
subsetEquality(transitiveCircularObjA2, transitiveCircularObjA1),
).toBe(true);
expect(
subsetEquality(transitiveCircularObjB, transitiveCircularObjA1),
).toBe(false);
expect(
subsetEquality(primitiveInsteadOfRef, transitiveCircularObjA1),
).toBe(false);
});
});
});

describe('iterableEquality', () => {
Expand Down
38 changes: 29 additions & 9 deletions packages/expect/src/utils.ts
View file
Open in desktop
Expand Up @@ -268,16 +268,36 @@ export const subsetEquality = (
object: any,
subset: any,
): undefined | boolean => {
if (!isObjectWithKeys(subset)) {
return undefined;
}
// subsetEquality needs to keep track of the references
// it has already visited to avoid infinite loops in case
// there are circular references in the subset passed to it.
const subsetEqualityWithContext = (
seenReferences: WeakMap<object, boolean> = new WeakMap(),
) => (object: any, subset: any): undefined | boolean => {
if (!isObjectWithKeys(subset)) {
return undefined;
}

return Object.keys(subset).every(
key =>
object != null &&
hasOwnProperty(object, key) &&
equals(object[key], subset[key], [iterableEquality, subsetEquality]),
);
return Object.keys(subset).every(key => {
if (isObjectWithKeys(subset[key])) {
if (seenReferences.get(subset[key])) {
pedrottimark marked this conversation as resolved.
Show resolved Hide resolved
return equals(object[key], subset[key], [iterableEquality]);
}
seenReferences.set(subset[key], true);
}

return (
object != null &&
hasOwnProperty(object, key) &&
equals(object[key], subset[key], [
iterableEquality,
subsetEqualityWithContext(seenReferences),
])
);
});
};

return subsetEqualityWithContext()(object, subset);
};

export const typeEquality = (a: any, b: any) => {
Expand Down