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

fixed asymmetrical equality of cyclic objects #7730

Merged
merged 8 commits into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
- `[jest-runtime]` Exclude setup/teardown files from coverage report ([#7790](https://github.com/facebook/jest/pull/7790)
- `[babel-jest]` Throw an error if `babel-jest` tries to transform a file ignored by Babel ([#7797](https://github.com/facebook/jest/pull/7797))
- `[babel-plugin-jest-hoist]` Ignore TS type references when looking for out-of-scope references ([#7799](https://github.com/facebook/jest/pull/7799)
- `[expect]` fixed asymmetrical equality of cyclic objects ([#7730](https://github.com/facebook/jest/pull/7730))

### Chore & Maintenance

Expand Down
50 changes: 50 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -606,6 +606,56 @@ describe('.toEqual()', () => {
});
expect(actual).toEqual({x: 3});
});

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

To make explicit guarantee that the comparison is symmetrical, do we want to double up on all the assertions?

Like the pair in the last test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point.
I will add another assertion to the test cases in which cyclic objects have the same shape.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s be explicit both for positive .toEqual or negative .not.toEqual There have been multiple new issues this week with inconsistent asymmetrical negative assertions in expect package.


const c = {};
c.x = a;
const d = {};
d.x = b;
expect(c).toEqual(d);
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I like it.

});

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);
});
});
});

describe('.toBeInstanceOf()', () => {
Expand Down
20 changes: 11 additions & 9 deletions packages/expect/src/jasmineUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@ function asymmetricMatch(a: any, b: any) {
function eq(
a: any,
b: any,
aStack: any,
bStack: any,
customTesters: any,
aStack: Array<unknown>,
Copy link
Member

Choose a reason for hiding this comment

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

better typing makes me happy! 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, he committed it better than he found it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better typing makes me happy! 🙏

Same here 🙌

bStack: Array<unknown>,
customTesters: Array<Tester>,
hasKey: any,
): boolean {
var result = true;
Expand Down Expand Up @@ -149,17 +149,19 @@ function eq(
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`.
// Used to detect circular references.
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;
// 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;
}
}
// Add the first object to the stack of traversed objects.
aStack.push(a);
bStack.push(b);
var size = 0;
Expand Down Expand Up @@ -201,7 +203,7 @@ function eq(
return false;
}
}
// Remove the first object from the stack of traversed objects.
grosto marked this conversation as resolved.
Show resolved Hide resolved

aStack.pop();
bStack.pop();

Expand Down