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 for #6138: toEqual deep compare set values and map keys #6150

Merged
merged 2 commits into from
May 22, 2018
Merged

Fix for #6138: toEqual deep compare set values and map keys #6150

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
600130a
Fix for #6138: Deep compare set values and map keys
chrisblossom May 7, 2018
602abe2
Merge branch 'master' into map-set-object-equal-fix
cpojer May 22, 2018
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,8 @@
configuration ([#5976](https://github.com/facebook/jest/pull/5976))
* `[website]` Fix website docs
([#5853](https://github.com/facebook/jest/pull/5853))
* `[expect]` Fix isEqual Set and Map to compare object values and keys
regardless of order ([#6150](https://github.com/facebook/jest/pull/6150))

### Chore & Maintenance

Expand Down
201 changes: 201 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2295,6 +2295,88 @@ Difference:
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Map {["v"] => 1}).toEqual(Map {["v"] => 2}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expected value to equal:
<green>Map {[\\"v\\"] => 2}</>
Received:
<red>Map {[\\"v\\"] => 1}</>

Difference:

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

<dim> Map {</>
<dim> Array [</>
<dim> \\"v\\",</>
<green>- ] => 2,</>
<red>+ ] => 1,</>
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Map {[1] => "one", [2] => "two", [3] => "three", [3] => "four"}).not.toEqual(Map {[3] => "three", [3] => "four", [2] => "two", [1] => "one"}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Map {[3] => \\"three\\", [3] => \\"four\\", [2] => \\"two\\", [1] => \\"one\\"}</>
Received:
<red>Map {[1] => \\"one\\", [2] => \\"two\\", [3] => \\"three\\", [3] => \\"four\\"}</>"
`;

exports[`.toEqual() {pass: false} expect(Map {[1] => "one", [2] => "two"}).not.toEqual(Map {[2] => "two", [1] => "one"}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Map {[2] => \\"two\\", [1] => \\"one\\"}</>
Received:
<red>Map {[1] => \\"one\\", [2] => \\"two\\"}</>"
`;

exports[`.toEqual() {pass: false} expect(Map {[1] => Map {[1] => "one"}, [2] => Map {[2] => "two"}}).not.toEqual(Map {[2] => Map {[2] => "two"}, [1] => Map {[1] => "one"}}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Map {[2] => Map {[2] => \\"two\\"}, [1] => Map {[1] => \\"one\\"}}</>
Received:
<red>Map {[1] => Map {[1] => \\"one\\"}, [2] => Map {[2] => \\"two\\"}}</>"
`;

exports[`.toEqual() {pass: false} expect(Map {[1] => Map {[1] => "one"}}).toEqual(Map {[1] => Map {[1] => "two"}}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expected value to equal:
<green>Map {[1] => Map {[1] => \\"two\\"}}</>
Received:
<red>Map {[1] => Map {[1] => \\"one\\"}}</>

Difference:

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

<yellow>@@ -2,8 +2,8 @@</>
<dim> Array [</>
<dim> 1,</>
<dim> ] => Map {</>
<dim> Array [</>
<dim> 1,</>
<green>- ] => \\"two\\",</>
<red>+ ] => \\"one\\",</>
<dim> },</>
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Map {{"a": 1} => "one", {"b": 2} => "two"}).not.toEqual(Map {{"b": 2} => "two", {"a": 1} => "one"}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Map {{\\"b\\": 2} => \\"two\\", {\\"a\\": 1} => \\"one\\"}</>
Received:
<red>Map {{\\"a\\": 1} => \\"one\\", {\\"b\\": 2} => \\"two\\"}</>"
`;

exports[`.toEqual() {pass: false} expect(Map {}).not.toEqual(Map {}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expand Down Expand Up @@ -2354,6 +2436,92 @@ Difference:
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Map {1 => ["one"], 2 => ["two"]}).not.toEqual(Map {2 => ["two"], 1 => ["one"]}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Map {2 => [\\"two\\"], 1 => [\\"one\\"]}</>
Received:
<red>Map {1 => [\\"one\\"], 2 => [\\"two\\"]}</>"
`;

exports[`.toEqual() {pass: false} expect(Set {[1], [2], [3], [3]}).not.toEqual(Set {[3], [3], [2], [1]}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Set {[3], [3], [2], [1]}</>
Received:
<red>Set {[1], [2], [3], [3]}</>"
`;

exports[`.toEqual() {pass: false} expect(Set {[1], [2]}).not.toEqual(Set {[2], [1]}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Set {[2], [1]}</>
Received:
<red>Set {[1], [2]}</>"
`;

exports[`.toEqual() {pass: false} expect(Set {[1], [2]}).toEqual(Set {[1], [2], [2]}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expected value to equal:
<green>Set {[1], [2], [2]}</>
Received:
<red>Set {[1], [2]}</>

Difference:

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

<yellow>@@ -3,9 +3,6 @@</>
<dim> 1,</>
<dim> ],</>
<dim> Array [</>
<dim> 2,</>
<dim> ],</>
<green>- Array [</>
<green>- 2,</>
<green>- ],</>
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Set {[1], [2]}).toEqual(Set {[1], [2], [3]}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expected value to equal:
<green>Set {[1], [2], [3]}</>
Received:
<red>Set {[1], [2]}</>

Difference:

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

<yellow>@@ -3,9 +3,6 @@</>
<dim> 1,</>
<dim> ],</>
<dim> Array [</>
<dim> 2,</>
<dim> ],</>
<green>- Array [</>
<green>- 3,</>
<green>- ],</>
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Set {{"a": 1}, {"b": 2}}).not.toEqual(Set {{"b": 2}, {"a": 1}}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Set {{\\"b\\": 2}, {\\"a\\": 1}}</>
Received:
<red>Set {{\\"a\\": 1}, {\\"b\\": 2}}</>"
`;

exports[`.toEqual() {pass: false} expect(Set {}).not.toEqual(Set {}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expand Down Expand Up @@ -2421,6 +2589,39 @@ Difference:
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(Set {Set {[1]}, Set {[2]}}).not.toEqual(Set {Set {[2]}, Set {[1]}}) 1`] = `
"<dim>expect(</><red>received</><dim>).not.toEqual(</><green>expected</><dim>)</>

Expected value to not equal:
<green>Set {Set {[2]}, Set {[1]}}</>
Received:
<red>Set {Set {[1]}, Set {[2]}}</>"
`;

exports[`.toEqual() {pass: false} expect(Set {Set {1}, Set {2}}).toEqual(Set {Set {1}, Set {3}}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expected value to equal:
<green>Set {Set {1}, Set {3}}</>
Received:
<red>Set {Set {1}, Set {2}}</>

Difference:

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

<dim> Set {</>
<dim> Set {</>
<dim> 1,</>
<dim> },</>
<dim> Set {</>
<green>- 3,</>
<red>+ 2,</>
<dim> },</>
<dim> }</>"
`;

exports[`.toEqual() {pass: false} expect(false).toEqual(ObjectContaining {"a": 2}) 1`] = `
"<dim>expect(</><red>received</><dim>).toEqual(</><green>expected</><dim>)</>

Expand Down
38 changes: 38 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,12 +247,23 @@ describe('.toEqual()', () => {
[new Map(), new Set()],
[new Set([1, 2]), new Set()],
[new Set([1, 2]), new Set([1, 2, 3])],
[new Set([[1], [2]]), new Set([[1], [2], [3]])],
[new Set([[1], [2]]), new Set([[1], [2], [2]])],
Copy link
Contributor

@mattphillips mattphillips May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It might be worth adding a Set of Sets in these tests i.e.

    [
      new Set([new Set([1]), new Set([2])]),
      new Set([new Set([1]), new Set([3])]),
    ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Added requested tests for both Map and Set for .toEqual and not.toEqual.

[
new Set([new Set([1]), new Set([2])]),
new Set([new Set([1]), new Set([3])]),
],
[Immutable.Set([1, 2]), Immutable.Set()],
[Immutable.Set([1, 2]), Immutable.Set([1, 2, 3])],
[Immutable.OrderedSet([1, 2]), Immutable.OrderedSet([2, 1])],
[new Map([[1, 'one'], [2, 'two']]), new Map([[1, 'one']])],
[new Map([['a', 0]]), new Map([['b', 0]])],
[new Map([['v', 1]]), new Map([['v', 2]])],
[new Map([[['v'], 1]]), new Map([[['v'], 2]])],
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above it might be worth testing against a Map of Maps

    [
      new Map([[[1], new Map([[[1], 'one']])]]),
      new Map([[[1], new Map([[[1], 'two']])]]),
    ]

[
new Map([[[1], new Map([[[1], 'one']])]]),
new Map([[[1], new Map([[[1], 'two']])]]),
],
[Immutable.Map({a: 0}), Immutable.Map({b: 0})],
[Immutable.Map({v: 1}), Immutable.Map({v: 2})],
[
Expand Down Expand Up @@ -304,6 +315,13 @@ describe('.toEqual()', () => {
[new Set(), new Set()],
[new Set([1, 2]), new Set([1, 2])],
[new Set([1, 2]), new Set([2, 1])],
[new Set([[1], [2]]), new Set([[2], [1]])],
[
new Set([new Set([[1]]), new Set([[2]])]),
new Set([new Set([[2]]), new Set([[1]])]),
],
[new Set([[1], [2], [3], [3]]), new Set([[3], [3], [2], [1]])],
[new Set([{a: 1}, {b: 2}]), new Set([{b: 2}, {a: 1}])],
[Immutable.Set(), Immutable.Set()],
[Immutable.Set([1, 2]), Immutable.Set([1, 2])],
[Immutable.Set([1, 2]), Immutable.Set([2, 1])],
Expand All @@ -312,6 +330,26 @@ describe('.toEqual()', () => {
[new Map(), new Map()],
[new Map([[1, 'one'], [2, 'two']]), new Map([[1, 'one'], [2, 'two']])],
[new Map([[1, 'one'], [2, 'two']]), new Map([[2, 'two'], [1, 'one']])],
[
new Map([[[1], 'one'], [[2], 'two'], [[3], 'three'], [[3], 'four']]),
new Map([[[3], 'three'], [[3], 'four'], [[2], 'two'], [[1], 'one']]),
],
[
new Map([[[1], new Map([[[1], 'one']])], [[2], new Map([[[2], 'two']])]]),
new Map([[[2], new Map([[[2], 'two']])], [[1], new Map([[[1], 'one']])]]),
],
[
new Map([[[1], 'one'], [[2], 'two']]),
new Map([[[2], 'two'], [[1], 'one']]),
],
[
new Map([[{a: 1}, 'one'], [{b: 2}, 'two']]),
new Map([[{b: 2}, 'two'], [{a: 1}, 'one']]),
],
[
new Map([[1, ['one']], [2, ['two']]]),
new Map([[2, ['two']], [1, ['one']]]),
],
[Immutable.Map(), Immutable.Map()],
[
Immutable.Map()
Expand Down
34 changes: 30 additions & 4 deletions packages/expect/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,18 @@ export const iterableEquality = (a: any, b: any) => {
let allFound = true;
for (const aValue of a) {
if (!b.has(aValue)) {
allFound = false;
break;
let has = false;
Copy link
Contributor

@mattphillips mattphillips May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I like the look of this 😄

I know the current approach is to use the for...of loop technique, I find this quite hard to reason about (although not everyone might agree with me 😉)

How about something like:

    ...

    } else if (isA('Set', a) || isImmutableUnorderedSet(a)) {

      const arrayA = [...a];
      return [...b].every(item => a.has(item) || contains(arrayA, item));

    } else if (isA('Map', a) || isImmutableUnorderedKeyed(a)) {

      const arrayA = [...a];
      return [...b].every(
        ([name, item]) =>
          (a.has(name) && equals(item, a.get(name), [iterableEquality])) ||
          contains(arrayA, [name, item]),
      );
    }

With a contains function of:

  const contains = (list, value) => {
    return list.findIndex(item => equals(item, value, [iterableEquality])) > -1;
  };

I'm not sure if there is any perf difference of the two implementations.

What do you think?

Copy link
Contributor Author

@chrisblossom chrisblossom May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It seems a bit more convoluted to me, but admittedly I am not very familiar with .every (I do understand what it is doing). I think confusing / magic part to me is the spreading of Map and Set, particularly inside contains.

That being said, I don't think the for...of implementation is that straight forward either.

From your example, the following is a bit more readable to me:

const contains = (list, value) => {
  const index = list.findIndex(item => equals(item, value, [iterableEquality]));

  return index > -1;
};

...
    } else if (isA('Set', a) || isImmutableUnorderedSet(a)) {
      return [...b].every(item => {
        if (a.has(item)) {
          return true;
        }

        return contains([...a], item);
      });
    } else if (isA('Map', a) || isImmutableUnorderedKeyed(a)) {
      return [...b].every(([name, item]) => {
        if (a.has(name) && equals(item, a.get(name), [iterableEquality])) {
          return true;
        }

        return contains([...a], [name, item]);
      });
    }
...

I don't have a big opinion either way. Just let me know whatever changes I should make.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its fine to leave it as for...of actually, it works and we have the tests covering the behaviour if it needs changing in the future 😄 great job with your first contribution dude!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Also, thanks for the code review. Appreciate it!

for (const bValue of b) {
const isEqual = equals(aValue, bValue, [iterableEquality]);
if (isEqual === true) {
has = true;
}
}

if (has === false) {
allFound = false;
break;
}
}
}
if (allFound) {
Expand All @@ -142,8 +152,24 @@ export const iterableEquality = (a: any, b: any) => {
!b.has(aEntry[0]) ||
!equals(aEntry[1], b.get(aEntry[0]), [iterableEquality])
) {
allFound = false;
break;
let has = false;
for (const bEntry of b) {
const matchedKey = equals(aEntry[0], bEntry[0], [iterableEquality]);

let matchedValue = false;
if (matchedKey === true) {
matchedValue = equals(aEntry[1], bEntry[1], [iterableEquality]);
}

if (matchedValue === true) {
has = true;
}
}

if (has === false) {
allFound = false;
break;
}
}
}
if (allFound) {
Expand Down