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

[chrisblossom]

Summary

Fix for #6138.

Currently using toEqual, Set and Map compare values instead of order, but if the values / keys are objects order matters. This PR fixes toEqual so values / keys as objects compare values instead of order. See also: #4303

Test plan

Added tests to packages/expect/src/__tests__/matchers.test.js and ran the command:
node ./packages/jest-cli/bin/jest.js ./packages/expect/src/__tests__/matchers.test.js

[codecov-io]

Codecov Report

Merging #6150 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #6150   +/-   ##
=======================================
  Coverage   64.19%   64.19%           
=======================================
  Files         219      219           
  Lines        8423     8423           
  Branches        4        4           
=======================================
  Hits         5407     5407           
  Misses       3015     3015           
  Partials        1        1

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8cfaa5...600130a. Read the comment docs.

[mattphillips]

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])]),
    ]

[chrisblossom]

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

[mattphillips]

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']])]]),
    ]

[mattphillips]

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?

[chrisblossom]

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.

[mattphillips]

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!

[chrisblossom]

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

[SimenB]

This looks great to me, thanks!

[cpojer]

cc @mjesun to consider this may cause some trouble when rolling out at FB.

[mjesun]

I'm going to release charlie.3 with the change and test internally

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.