From 33ea4696f926f93f1a42d8c5764a9afea9794c55 Mon Sep 17 00:00:00 2001 From: rostog Date: Sun, 27 Jan 2019 16:46:08 +0100 Subject: [PATCH 1/6] fixed assymetrical equality of cyclic objects --- .../expect/src/__tests__/matchers.test.js | 9 ++++ packages/expect/src/jasmineUtils.js | 47 ++++++++++++------- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index bff61b7edc12..75eebe1b2db0 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -587,6 +587,15 @@ describe('.toEqual()', () => { }); expect(actual).toEqual({x: 3}); }); + + test('cyclic object equality is symmetrical', () => { + const a = {}; + a.x = {x: a}; + const b = {}; + b.x = b; + expect(a).not.toEqual(b); + expect(b).not.toEqual(a); + }); }); describe('.toBeInstanceOf()', () => { diff --git a/packages/expect/src/jasmineUtils.js b/packages/expect/src/jasmineUtils.js index a1b88aac8f5a..b5ca399fe68b 100644 --- a/packages/expect/src/jasmineUtils.js +++ b/packages/expect/src/jasmineUtils.js @@ -35,7 +35,7 @@ export function equals( strictCheck?: boolean, ): boolean { customTesters = customTesters || []; - return eq(a, b, [], [], customTesters, strictCheck ? hasKey : hasDefinedKey); + return eq(a, b, undefined, customTesters, strictCheck ? hasKey : hasDefinedKey); } function isAsymmetric(obj) { @@ -62,7 +62,7 @@ function asymmetricMatch(a, b) { // Equality function lovingly adapted from isEqual in // [Underscore](http://underscorejs.org) -function eq(a, b, aStack, bStack, customTesters, hasKey): boolean { +function eq(a, b, memos, customTesters, hasKey): boolean { var result = true; var asymmetricResult = asymmetricMatch(a, b); @@ -145,19 +145,30 @@ function eq(a, b, aStack, bStack, customTesters, hasKey): boolean { return false; } - // Assume equality for cyclic structures. The algorithm for detecting cyclic - // structures is adapted from ES 5.1 section 15.12.3, abstract operation `JO`. - var length = aStack.length; - while (length--) { - // Linear search. Performance is inversely proportional to the number of - // unique nested structures. - if (aStack[length] == a) { - return bStack[length] == b; + // Use memos to handle cycles. + if (memos === undefined) { + memos = { + a: new Map(), + b: new Map(), + position: 0 + }; + } else { + // We prevent up to two map.has(x) calls by directly retrieving the value + // and checking for undefined. The map can only contain numbers, so it is + // safe to check for undefined only. + const bMemoA = memos.a.get(a); + if (bMemoA !== undefined) { + const bMemoB = memos.b.get(b); + if (bMemoB !== undefined) { + return bMemoA === bMemoB; + } } + memos.position++; } - // Add the first object to the stack of traversed objects. - aStack.push(a); - bStack.push(b); + + memos.a.set(a, memos.position); + memos.b.set(b, memos.position); + var size = 0; // Recursively compare objects and arrays. // Compare array lengths to determine if a deep comparison is necessary. @@ -168,7 +179,7 @@ function eq(a, b, aStack, bStack, customTesters, hasKey): boolean { } while (size--) { - result = eq(a[size], b[size], aStack, bStack, customTesters, hasKey); + result = eq(a[size], b[size], memos, customTesters, hasKey); if (!result) { return false; } @@ -191,15 +202,15 @@ function eq(a, b, aStack, bStack, customTesters, hasKey): boolean { // Deep compare each member result = hasKey(b, key) && - eq(a[key], b[key], aStack, bStack, customTesters, hasKey); + eq(a[key], b[key], memos, customTesters, hasKey); if (!result) { return false; } } - // Remove the first object from the stack of traversed objects. - aStack.pop(); - bStack.pop(); + + memos.a.delete(a); + memos.b.delete(b); return result; } From 73024a2b8f10ff74b2e9da3c1b725a432f2c23b5 Mon Sep 17 00:00:00 2001 From: rostog Date: Mon, 28 Jan 2019 10:47:57 +0100 Subject: [PATCH 2/6] updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e6077184f0f3..d3d5697574e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixes +- `[expect]` fixed asymmetrical equality of cyclic objects ([#7730](https://github.com/facebook/jest/pull/7730)) - `[jest-cli]` Break dependency cycle when using Jest programmatically ([#7707](https://github.com/facebook/jest/pull/7707)) - `[jest-config]` Extract setupFilesAfterEnv from preset ([#7724](https://github.com/facebook/jest/pull/7724)) From 93613a0c2ae3e6c85410c8142b9a0e3a814a750a Mon Sep 17 00:00:00 2001 From: rostog Date: Tue, 5 Feb 2019 14:37:55 +0100 Subject: [PATCH 3/6] added more test cases and changed algorithm --- CHANGELOG.md | 3 ++- .../expect/src/__tests__/matchers.test.js | 27 +++++++++++++++++++ packages/expect/src/jasmineUtils.js | 8 +++--- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d3d5697574e7..38d1547ff619 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,9 +4,10 @@ ### Fixes -- `[expect]` fixed asymmetrical equality of cyclic objects ([#7730](https://github.com/facebook/jest/pull/7730)) - `[jest-cli]` Break dependency cycle when using Jest programmatically ([#7707](https://github.com/facebook/jest/pull/7707)) - `[jest-config]` Extract setupFilesAfterEnv from preset ([#7724](https://github.com/facebook/jest/pull/7724)) +- `[expect]` fixed asymmetrical equality of cyclic objects ([#7730](https://github.com/facebook/jest/pull/7730)) + ### Chore & Maintenance diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index 75eebe1b2db0..8b5ccf492083 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -588,6 +588,33 @@ describe('.toEqual()', () => { expect(actual).toEqual({x: 3}); }); + test('cyclic object properties with the same circularity are equal', () => { + const a = {}; + a.x = a; + const b = {}; + b.x = b; + expect(a).toEqual(b); + }); + + test('cyclic objects properties with different circularity are not equal', () => { + const a = {}; + a.x = {y: a}; + const b = {}; + const bx = {}; + b.x = bx; + bx.y = bx; + expect(a).not.toEqual(b); + }); + + test('cyclic objects are not equal if circularity is not on the same property', () => { + const a = {}; + const b = {}; + a.a = a; + b.a = {}; + b.a.a = a; + expect(a).not.toEqual(b); + }); + test('cyclic object equality is symmetrical', () => { const a = {}; a.x = {x: a}; diff --git a/packages/expect/src/jasmineUtils.js b/packages/expect/src/jasmineUtils.js index b5ca399fe68b..4bd1439b0cb5 100644 --- a/packages/expect/src/jasmineUtils.js +++ b/packages/expect/src/jasmineUtils.js @@ -157,11 +157,9 @@ function eq(a, b, memos, customTesters, hasKey): boolean { // and checking for undefined. The map can only contain numbers, so it is // safe to check for undefined only. const bMemoA = memos.a.get(a); - if (bMemoA !== undefined) { - const bMemoB = memos.b.get(b); - if (bMemoB !== undefined) { - return bMemoA === bMemoB; - } + const bMemoB = memos.b.get(b); + if (bMemoA !== undefined || bMemoB !== undefined) { + return bMemoA === bMemoB; } memos.position++; } From fc8b1beaa250cf864fd73dfed46807b2a1617e49 Mon Sep 17 00:00:00 2001 From: rostog Date: Tue, 26 Feb 2019 21:46:26 +0100 Subject: [PATCH 4/6] reverted to the old algorith and added additional check --- .../expect/src/__tests__/matchers.test.js | 82 +++++++++++-------- packages/expect/src/jasmineUtils.ts | 49 +++++------ 2 files changed, 69 insertions(+), 62 deletions(-) diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index 9eaf50c44938..f550f8367a89 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -607,40 +607,54 @@ describe('.toEqual()', () => { expect(actual).toEqual({x: 3}); }); - test('cyclic object properties with the same circularity are equal', () => { - const a = {}; - a.x = a; - const b = {}; - b.x = b; - expect(a).toEqual(b); - }); - - test('cyclic objects properties with different circularity are not equal', () => { - const a = {}; - a.x = {y: a}; - const b = {}; - const bx = {}; - b.x = bx; - bx.y = bx; - expect(a).not.toEqual(b); - }); - - test('cyclic objects are not equal if circularity is not on the same property', () => { - const a = {}; - const b = {}; - a.a = a; - b.a = {}; - b.a.a = a; - expect(a).not.toEqual(b); - }); - - test('cyclic object equality is symmetrical', () => { - const a = {}; - a.x = {x: a}; - const b = {}; - b.x = b; - expect(a).not.toEqual(b); - expect(b).not.toEqual(a); + describe('cyclic object equality', () => { + test('properties with the same circularity are equal', () => { + const a = {}; + a.x = a; + const b = {}; + b.x = b; + expect(a).toEqual(b); + + const c = {}; + c.x = a; + const d = {}; + d.x = b; + expect(c).toEqual(d); + }); + + test('properties with different circularity are not equal', () => { + const a = {}; + a.x = {y: a}; + const b = {}; + const bx = {}; + b.x = bx; + bx.y = bx; + expect(a).not.toEqual(b); + + const c = {}; + c.x = a; + const d = {}; + d.x = b; + expect(c).not.toEqual(d); + }); + + test('are not equal if circularity is not on the same property', () => { + const a = {}; + const b = {}; + a.a = a; + b.a = {}; + b.a.a = a; + expect(a).not.toEqual(b); + }); + + test('equality is symmetrical', () => { + const a = {}; + a.x = {x: a}; + const b = {}; + b.x = b; + expect(a).not.toEqual(b); + expect(b).not.toEqual(a); + }); }); }); diff --git a/packages/expect/src/jasmineUtils.ts b/packages/expect/src/jasmineUtils.ts index 5afb192f3e97..83d3750378f4 100644 --- a/packages/expect/src/jasmineUtils.ts +++ b/packages/expect/src/jasmineUtils.ts @@ -34,7 +34,7 @@ export function equals( strictCheck?: boolean, ): boolean { customTesters = customTesters || []; - return eq(a, b, undefined, customTesters, strictCheck ? hasKey : hasDefinedKey); + return eq(a, b, [], [], customTesters, strictCheck ? hasKey : hasDefinedKey); } function isAsymmetric(obj: any) { @@ -64,9 +64,9 @@ function asymmetricMatch(a: any, b: any) { function eq( a: any, b: any, - aStack: any, - bStack: any, - customTesters: any, + aStack: Array, + bStack: Array, + customTesters: Array, hasKey: any, ): boolean { var result = true; @@ -149,28 +149,21 @@ function eq( return false; } - // Use memos to handle cycles. - if (memos === undefined) { - memos = { - a: new Map(), - b: new Map(), - position: 0 - }; - } else { - // We prevent up to two map.has(x) calls by directly retrieving the value - // and checking for undefined. The map can only contain numbers, so it is - // safe to check for undefined only. - const bMemoA = memos.a.get(a); - const bMemoB = memos.b.get(b); - if (bMemoA !== undefined || bMemoB !== undefined) { - return bMemoA === bMemoB; + // Used to detect circular references. + var length = aStack.length; + while (length--) { + // Linear search. Performance is inversely proportional to the number of + // unique nested structures. + // circular references at same depth are equal + // circular reference is not equal to non-circular one + if (aStack[length] === a) { + return bStack[length] === b; + } else if (bStack[length] === b) { + return aStack[length] === a; } - memos.position++; } - - memos.a.set(a, memos.position); - memos.b.set(b, memos.position); - + aStack.push(a); + bStack.push(b); var size = 0; // Recursively compare objects and arrays. // Compare array lengths to determine if a deep comparison is necessary. @@ -181,7 +174,7 @@ function eq( } while (size--) { - result = eq(a[size], b[size], memos, customTesters, hasKey); + result = eq(a[size], b[size], aStack, bStack, customTesters, hasKey); if (!result) { return false; } @@ -204,15 +197,15 @@ function eq( // Deep compare each member result = hasKey(b, key) && - eq(a[key], b[key], memos, customTesters, hasKey); + eq(a[key], b[key], aStack, bStack, customTesters, hasKey); if (!result) { return false; } } - memos.a.delete(a); - memos.b.delete(b); + aStack.pop(); + bStack.pop(); return result; } From 2304df87a81b18fbecfbcdc4bcdd2011bc5d8c14 Mon Sep 17 00:00:00 2001 From: rostog Date: Tue, 26 Feb 2019 22:02:18 +0100 Subject: [PATCH 5/6] removed unnecessary check --- packages/expect/src/jasmineUtils.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/expect/src/jasmineUtils.ts b/packages/expect/src/jasmineUtils.ts index 83d3750378f4..f16bc5aeaa64 100644 --- a/packages/expect/src/jasmineUtils.ts +++ b/packages/expect/src/jasmineUtils.ts @@ -159,9 +159,10 @@ function eq( if (aStack[length] === a) { return bStack[length] === b; } else if (bStack[length] === b) { - return aStack[length] === a; + return false; } } + // Add the first object to the stack of traversed objects. aStack.push(a); bStack.push(b); var size = 0; @@ -203,7 +204,7 @@ function eq( return false; } } - + // Remove the first object from the stack of traversed objects. aStack.pop(); bStack.pop(); From f2a4f01ee4b33d60e5d3a909ccdeff4f38d7c398 Mon Sep 17 00:00:00 2001 From: rostog Date: Tue, 26 Feb 2019 22:54:24 +0100 Subject: [PATCH 6/6] added more assertions to check for symmetrical equality --- .../expect/src/__tests__/matchers.test.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/expect/src/__tests__/matchers.test.js b/packages/expect/src/__tests__/matchers.test.js index f550f8367a89..cff90804df79 100644 --- a/packages/expect/src/__tests__/matchers.test.js +++ b/packages/expect/src/__tests__/matchers.test.js @@ -614,12 +614,14 @@ describe('.toEqual()', () => { const b = {}; b.x = b; expect(a).toEqual(b); + expect(b).toEqual(a); const c = {}; c.x = a; const d = {}; d.x = b; expect(c).toEqual(d); + expect(d).toEqual(c); }); test('properties with different circularity are not equal', () => { @@ -630,12 +632,14 @@ describe('.toEqual()', () => { b.x = bx; bx.y = bx; expect(a).not.toEqual(b); + expect(b).not.toEqual(a); const c = {}; c.x = a; const d = {}; d.x = b; expect(c).not.toEqual(d); + expect(d).not.toEqual(c); }); test('are not equal if circularity is not on the same property', () => { @@ -645,15 +649,14 @@ describe('.toEqual()', () => { b.a = {}; b.a.a = a; expect(a).not.toEqual(b); - }); - - test('equality is symmetrical', () => { - const a = {}; - a.x = {x: a}; - const b = {}; - b.x = b; - expect(a).not.toEqual(b); expect(b).not.toEqual(a); + + const c = {}; + c.x = {x: c}; + const d = {}; + d.x = d; + expect(c).not.toEqual(d); + expect(d).not.toEqual(c); }); }); });