From 2c6eb483ec569a6dea94770b7e7de4c8771d0635 Mon Sep 17 00:00:00 2001 From: Lucas Fernandes da Costa Date: Sat, 13 Jul 2019 18:09:00 +0100 Subject: [PATCH 1/2] fix: handle circular references correctly in objects (closes #8663) --- CHANGELOG.md | 1 + .../__snapshots__/matchers.test.js.snap | 98 ++++++++++++++ .../expect/src/__tests__/matchers.test.js | 126 +++++++++++++++--- packages/expect/src/__tests__/utils.test.js | 54 ++++++++ packages/expect/src/utils.ts | 38 ++++-- 5 files changed, 286 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f9aa69ebc30..24eb8a965277 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap index 38ef42eebc21..3bebdcd75fa7 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -4092,6 +4092,104 @@ Expected: not Set {2, 1} Received: 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`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + + Object { +- \\"a\\": \\"hello\\", +- \\"ref\\": [Circular], ++ \\"ref\\": \\"not a ref\\", + }" +`; + +exports[`toMatchObject() circular references simple circular references {pass: false} expect({}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + +- Object { +- \\"a\\": \\"hello\\", +- \\"ref\\": [Circular], +- } ++ Object {}" +`; + +exports[`toMatchObject() circular references simple circular references {pass: true} expect({"a": "hello", "ref": [Circular]}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = ` +"expect(received).not.toMatchObject(expected) + +Expected: not {\\"a\\": \\"hello\\", \\"ref\\": [Circular]}" +`; + +exports[`toMatchObject() circular references simple circular references {pass: true} expect({"a": "hello", "ref": [Circular]}).toMatchObject({}) 1`] = ` +"expect(received).not.toMatchObject(expected) + +Expected: not {} +Received: {\\"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"`; + +exports[`toMatchObject() circular references transitive circular references {pass: false} expect({"nestedObj": {"parentObj": "not the parent ref"}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + + Object { +- \\"a\\": \\"hello\\", + \\"nestedObj\\": Object { +- \\"parentObj\\": [Circular], ++ \\"parentObj\\": \\"not the parent ref\\", + }, + }" +`; + +exports[`toMatchObject() circular references transitive circular references {pass: false} expect({}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + +- Object { +- \\"a\\": \\"hello\\", +- \\"nestedObj\\": Object { +- \\"parentObj\\": [Circular], +- }, +- } ++ Object {}" +`; + +exports[`toMatchObject() circular references transitive circular references {pass: true} expect({"a": "hello", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = ` +"expect(received).not.toMatchObject(expected) + +Expected: not {\\"a\\": \\"hello\\", \\"nestedObj\\": {\\"parentObj\\": [Circular]}}" +`; + +exports[`toMatchObject() circular references transitive circular references {pass: true} expect({"a": "hello", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({}) 1`] = ` +"expect(received).not.toMatchObject(expected) + +Expected: not {} +Received: {\\"a\\": \\"hello\\", \\"nestedObj\\": {\\"parentObj\\": [Circular]}}" +`; + +exports[`toMatchObject() does not match properties up in the prototype chain 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + + Object { + \\"other\\": \\"child\\", +- \\"ref\\": [Circular], + }" +`; + exports[`toMatchObject() throws expect("44").toMatchObject({}) 1`] = ` "expect(received).toMatchObject(expected) diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index 9b5661339c0f..d17957ad1d40 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -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'}}], @@ -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)}], @@ -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, {}], @@ -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(); + }); }); diff --git a/packages/expect/src/__tests__/utils.test.js b/packages/expect/src/__tests__/utils.test.js index 0850053b120a..f12eb0e667f6 100644 --- a/packages/expect/src/__tests__/utils.test.js +++ b/packages/expect/src/__tests__/utils.test.js @@ -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', () => { diff --git a/packages/expect/src/utils.ts b/packages/expect/src/utils.ts index f8aa8aec01d3..8b76bde631d3 100644 --- a/packages/expect/src/utils.ts +++ b/packages/expect/src/utils.ts @@ -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 = 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])) { + 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) => { From 1f7bf0a3c08779e6edc20d27bcb52614f866a738 Mon Sep 17 00:00:00 2001 From: Lucas Fernandes da Costa Date: Wed, 17 Jul 2019 21:20:10 +0100 Subject: [PATCH 2/2] fix: handling circular references properly in getObjectSubset (#8663) --- .../__snapshots__/matchers.test.js.snap | 28 +++++++- packages/expect/src/__tests__/utils.test.js | 68 +++++++++++++++++++ packages/expect/src/utils.ts | 32 +++++---- 3 files changed, 112 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 3bebdcd75fa7..e50e796cb05f 100644 --- a/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap +++ b/packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap @@ -4092,7 +4092,18 @@ Expected: not Set {2, 1} Received: 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({"a": "hello", "ref": [Circular]}).toMatchObject({"a": "world", "ref": [Circular]}) 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + + Object { +- \\"a\\": \\"world\\", ++ \\"a\\": \\"hello\\", + \\"ref\\": [Circular], + }" +`; exports[`toMatchObject() circular references simple circular references {pass: false} expect({"ref": "not a ref"}).toMatchObject({"a": "hello", "ref": [Circular]}) 1`] = ` "expect(received).toMatchObject(expected) @@ -4133,7 +4144,20 @@ Expected: not {} Received: {\\"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"`; +exports[`toMatchObject() circular references transitive circular references {pass: false} expect({"a": "world", "nestedObj": {"parentObj": [Circular]}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = ` +"expect(received).toMatchObject(expected) + +- Expected ++ Received + + Object { +- \\"a\\": \\"hello\\", ++ \\"a\\": \\"world\\", + \\"nestedObj\\": Object { + \\"parentObj\\": [Circular], + }, + }" +`; exports[`toMatchObject() circular references transitive circular references {pass: false} expect({"nestedObj": {"parentObj": "not the parent ref"}}).toMatchObject({"a": "hello", "nestedObj": {"parentObj": [Circular]}}) 1`] = ` "expect(received).toMatchObject(expected) diff --git a/packages/expect/src/__tests__/utils.test.js b/packages/expect/src/__tests__/utils.test.js index f12eb0e667f6..b20b3db68700 100644 --- a/packages/expect/src/__tests__/utils.test.js +++ b/packages/expect/src/__tests__/utils.test.js @@ -164,6 +164,74 @@ describe('getObjectSubset()', () => { }, ); }); + + describe('calculating subsets of objects with circular references', () => { + test('simple circular references', () => { + const nonCircularObj = {a: 'world', b: 'something'}; + + const circularObjA = {a: 'hello'}; + circularObjA.ref = circularObjA; + + const circularObjB = {a: 'world'}; + circularObjB.ref = circularObjB; + + const primitiveInsteadOfRef = {b: 'something'}; + primitiveInsteadOfRef.ref = 'not a ref'; + + const nonCircularRef = {b: 'something'}; + nonCircularRef.ref = {}; + + expect(getObjectSubset(circularObjA, nonCircularObj)).toEqual({ + a: 'hello', + }); + expect(getObjectSubset(nonCircularObj, circularObjA)).toEqual({ + a: 'world', + }); + + expect(getObjectSubset(circularObjB, circularObjA)).toEqual(circularObjB); + + expect(getObjectSubset(primitiveInsteadOfRef, circularObjA)).toEqual({ + ref: 'not a ref', + }); + expect(getObjectSubset(nonCircularRef, circularObjA)).toEqual({ + ref: {}, + }); + }); + + test('transitive circular references', () => { + const nonCircularObj = {a: 'world', b: 'something'}; + + const transitiveCircularObjA = {a: 'hello'}; + transitiveCircularObjA.nestedObj = {parentObj: transitiveCircularObjA}; + + const transitiveCircularObjB = {a: 'world'}; + transitiveCircularObjB.nestedObj = {parentObj: transitiveCircularObjB}; + + const primitiveInsteadOfRef = {}; + primitiveInsteadOfRef.nestedObj = {otherProp: 'not the parent ref'}; + + const nonCircularRef = {}; + nonCircularRef.nestedObj = {otherProp: {}}; + + expect(getObjectSubset(transitiveCircularObjA, nonCircularObj)).toEqual({ + a: 'hello', + }); + expect(getObjectSubset(nonCircularObj, transitiveCircularObjA)).toEqual({ + a: 'world', + }); + + expect( + getObjectSubset(transitiveCircularObjB, transitiveCircularObjA), + ).toEqual(transitiveCircularObjB); + + expect( + getObjectSubset(primitiveInsteadOfRef, transitiveCircularObjA), + ).toEqual({nestedObj: {otherProp: 'not the parent ref'}}); + expect(getObjectSubset(nonCircularRef, transitiveCircularObjA)).toEqual({ + nestedObj: {otherProp: {}}, + }); + }); + }); }); describe('emptyObject()', () => { diff --git a/packages/expect/src/utils.ts b/packages/expect/src/utils.ts index 8b76bde631d3..6e77197bf219 100644 --- a/packages/expect/src/utils.ts +++ b/packages/expect/src/utils.ts @@ -104,7 +104,11 @@ export const getPath = ( // Strip properties from object that are not present in the subset. Useful for // printing the diff for toMatchObject() without adding unrelated noise. -export const getObjectSubset = (object: any, subset: any): any => { +export const getObjectSubset = ( + object: any, + subset: any, + seenReferences: WeakMap = new WeakMap(), +): any => { if (Array.isArray(object)) { if (Array.isArray(subset) && subset.length === object.length) { return subset.map((sub: any, i: number) => @@ -113,18 +117,17 @@ export const getObjectSubset = (object: any, subset: any): any => { } } else if (object instanceof Date) { return object; - } else if ( - typeof object === 'object' && - object !== null && - typeof subset === 'object' && - subset !== null - ) { + } else if (isObject(object) && isObject(subset)) { const trimmed: any = {}; - Object.keys(subset) - .filter(key => hasOwnProperty(object, key)) - .forEach( - key => (trimmed[key] = getObjectSubset(object[key], subset[key])), - ); + seenReferences.set(object, trimmed); + + Object.keys(object) + .filter(key => hasOwnProperty(subset, key)) + .forEach(key => { + trimmed[key] = seenReferences.has(object[key]) + ? seenReferences.get(object[key]) + : getObjectSubset(object[key], subset[key], seenReferences); + }); if (Object.keys(trimmed).length > 0) { return trimmed; @@ -257,9 +260,10 @@ export const iterableEquality = ( return true; }; +const isObject = (a: any) => a !== null && typeof a === 'object'; + const isObjectWithKeys = (a: any) => - a !== null && - typeof a === 'object' && + isObject(a) && !(a instanceof Error) && !(a instanceof Array) && !(a instanceof Date);