toEqual not symmetric for Set

[dubzzz]

🐛 Bug Report

toEqual is not symmetric for Set:

const s1 = new Set([false, true]);
const s2 = new Set([new Boolean(true), new Boolean(true)]);

expect(s1).not.toEqual(s2); // success

expect(s2).not.toEqual(s1); // failure
//    expect(received).not.toEqual(expected)
//    Expected: Set {false, true}
//    Received: Set {{}, {}}

To Reproduce

Steps to reproduce the behavior: try the code snippet above.

Expected behavior

I would have expected to have the same result for expect(s1).not.toEqual(s2) and expect(s2).not.toEqual(s1).

Run npx envinfo --preset jest

Paste the results here:

$ npx envinfo --preset jest
npx : 1 installé(s) en 4.679s

  System:
    OS: Windows 10
    CPU: (8) x64 Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
  Binaries:
    Node: 10.12.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.10.1 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 6.7.0 - C:\Program Files\nodejs\npm.CMD

[dubzzz]

The logic doing the check for Array seems to be https://github.com/facebook/jest/blob/master/packages/expect/src/jasmineUtils.ts#L168-L182
There is certainly something similar to build to be able to deal with Set.

[pedrottimark]

Keep digging! An inconsistent result is a problem.

To decide on a solution, we need to think what a deep-equality comparison means for instances in sets (or tuples with instance keys in maps).

describe('Set', () => {
  test('primitives', () => {
    const set = new Set([false, true, false]);
    expect(set.size).toBe(2);
  });
  test('primitives versus instances', () => {
    const set = new Set([
      false,
      true,
      false,
      new Boolean(false),
      new Boolean(true),
      new Boolean(false)
    ]);
    expect(set.size).toBe(5);
    expect(set).toMatchSnapshot();
  });
});

Set {
  false,
  true,
  Boolean {},
  Boolean {},
  Boolean {},
}

[dubzzz]

Indeed the Set case is pretty funny ;)

Do we need to consider that Set([new Boolean(false), new Boolean(false)]) equals new Set([false, false])? The first one produces a set having two entries while the second one only has a single one.

Today, we do consider that [new Boolean(false), new Boolean(false)] equals [false, false] for arrays (if I am not wrong). So it might look logical that set would behave the same in some way.

But clearly the real issue above all is the symmetry. I would have expected to have a symmetrical expect(x).toEqual(y)

[pedrottimark]

The iterableEquality custom tester defined in utils.ts file has asymmetric code:

• Set https://github.com/facebook/jest/blob/master/packages/expect/src/utils.ts#L155-L158
• Map https://github.com/facebook/jest/blob/master/packages/expect/src/utils.ts#L155-L168

to call either has method (referential identity) or equals function (deep equality)

• Set needs has method in both directions
• Map needs same as above to compare keys and maybe deep equality to compare values

Here are a few quotes from https://leanpub.com/understandinges6/read#leanpub-auto-sets-and-maps

The conversion to the default string representation makes it difficult to use objects as keys [in classic object as a map].

Sets don’t coerce values to determine whether they’re the same. Internally, the comparison uses the Object.is() method. You can also add multiple objects to the set, and those objects will remain distinct.

The ECMAScript 6 Map type is an ordered list of key-value pairs, where the key and value can be any type. Key equivalence is determined by calling the Object.is() method. This is quite different from using object properties as keys, because object properties always coerce values to strings.

[dubzzz]

Thanks a lot for the entry points for the equality of two Set.
I just went through the code with the example of s1 and s2 given in the description.

Here are some of the quick remarks I have about the code section.

Remark 1

It seems that (1) https://github.com/facebook/jest/blob/master/packages/expect/src/utils.ts#L159-L178 is doing the same as (2) https://github.com/facebook/jest/blob/master/packages/expect/src/utils.ts#L212-L223.

Returning false at line 178 should be ok.

Remark 2

I think we could add a break in https://github.com/facebook/jest/blob/master/packages/expect/src/utils.ts#L165-L167
Indeed, we know that we've already found what we were looking for.

Analysis

In the case of equal(new Set([new Boolean(true), new Boolean(true)]), new Set([false, true])) the function returns true.

I went through the code with logs everywhere to track what was going on.
Here is basically what we have for this case:

• a and b have the same size - check ok
• we iterate over the values in a
• first value, true, is inside b - check ok
• second value, true, is inside b - check ok
• DONE

The real problem is that we check that all values within a are within b but not the contrary.
By the way, we also have the same issue for:

expect(new Set([{}, {}])).not.toEqual(new Set([{}, {a: 1}]));

A simple fix would be to do the symmetric check for b.
Would you be ok for such fix?

[dubzzz]

The underlying problem is that Set is using === (neither === nor Object.is because for Set NaN is NaN, 0 is -0) for its equality while Jest considers equals. In other words Set.prototype.size is the size of the Set according to === equality operator so Jest should be very careful with it: if all the elements of a are in b and a has the same size as b, it does not mean that a equals b :/

Possible fixes I can come with:

1. Iterate on a to check all its values are within b and do the same with b
2. or Copy b into an array, for any value in a find the first value in the array such as a Jest-equals b and remove it from the array

[grosto]

That's a great find.

I have been playing around with equals for the PR I am working on. I wanted to add some ideas regarding iterableEquality. They might not be directly related to this issue but might affect how we solve it :)

First of all, there is one more bug with this tester. The code below throws Maximum call stack size exceeded. Same is true for Map.

const a = new Set();
const b = new Set();
a.add(a);
b.add(b);
expect(a).toEqual(b);

@pedrottimark is there reason why iterableEquality is not part of equals? We are passing it in every equals call anyway. It's not really a custom tester either since Map and Set are part of the language and it simply checks for their equality(same for other iterables). if we make it part of equals we can also rely on already implemented cyclic object detection.

[jeysal]

For future reference: #8281 introduces Map/Set special handling because of this problem

[github-actions]

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

[github-actions]

This issue 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.