assert: deepEqual of two Sets with different content passes #13347

Activity

1. vsemozhetbyt commented on May 31, 2017

   

   vsemozhetbyt

   on May 31, 2017

   Last edited by vsemozhetbyt

   Contributor

   More actions

   I can reproduce.
   This fails:

   assert.deepEqual(new Set([{a: 1}, {a: 1}]), new Set([{b: 1}, {b: 2}]));
   assert.deepStrictEqual(new Set([{a: 1}, {a: 1}]), new Set([{b: 1}, {b: 2}]));

   This also fails:

   assert.deepEqual(new Set([{a: 1}]), new Set([{a: 2}]));
   assert.deepStrictEqual(new Set([{a: 1}]), new Set([{a: 2}]));
2. refack

   added

   assertIssues and PRs related to the assert subsystem.Issues and PRs related to the assert subsystem.

   confirmed-bugIssues and PRs for confirmed bugs.Issues and PRs for confirmed bugs.

   on May 31, 2017
3. vsemozhetbyt commented on May 31, 2017

   

   vsemozhetbyt

   on May 31, 2017

   Contributor

   More actions

   сс @nodejs/testing
4. vsemozhetbyt commented on May 31, 2017

   

   vsemozhetbyt

   on May 31, 2017

   Contributor

   More actions

   Also cc @josephg due to #12142
5. josephg commented on May 31, 2017

   

   josephg

   on May 31, 2017

   Last edited by josephg

   Contributor

   More actions

   Errr, yep this is a bug.

   assert.deepStrictEqual(new Set([{a: 1}, {a: 1}]), new Set([{a: 1}, {a: 2}]))
   

   There's two semantics going on here:

   1. In a set, these objects are different: {a:1} and {a:1} because they aren't references to the same object.
   2. In deep[Strict]Equal, they are the same.

   So you've made a set with two different objects that are considered identical by deepEqual. As far as deepEqual should be concerned, is this equivalent to a set with only one {a:1} in it? Its sort of weird.

   Anyway, after a long discussion deep equality comparison currently works by doing this:

   1. Assert the sets have identical .size
   2. For each item in set 1, find an equivalent item in set 2

   The .size comparison uses the semantics of sets - that is, internal reference equality. The pairwise item comparisons uses the semantics of deepEqual / deepStrictEqual. And hence the bug.

   We could fix it by changing the code to do it both ways around: (Edit: This is also buggy - see my comment below)

   1. Assert the sets have identical .size
   2. For each item in set 1, find an equivalent item in set 2
   3. For each item in set 2, find an equivalent item in set 1

   (... although then step 1 isn't needed anymore, and might cause more bugs?)

   Alternately, we could track which items of set 2 have already been consumed by the matching. So:

   1. Assert the sets have identical .size
   2. For each item in set 1, find an equivalent item in set 2 that hasn't already been used

   .. Which I think is correct.
6. josephg commented on May 31, 2017

   

   josephg

   on May 31, 2017

   Last edited by josephg

   Contributor

   More actions

   And, maps have the equivalent logic & same bug:

   assert.deepEqual(new Map([[{x:1}, 5], [{x:1}, 5]]), new Map([[{x:1}, 5], [{x:2}, 5]]))
   assert.deepStrictEqual(new Map([[{x:1}, 5], [{x:1}, 5]]), new Map([[{x:1}, 5], [{x:2}, 5]]))
7. vsemozhetbyt commented on May 31, 2017

   

   vsemozhetbyt

   on May 31, 2017

   Last edited by vsemozhetbyt

   Contributor

   More actions

   (... although then step 1 isn't needed anymore, and might cause more bugs?)

   Is it still needed with this approach to prevent this from being equal?

   assert.deepStrictEqual(new Set([{a: 1}, {a: 1}]), new Set([{a: 1}]))
8. josephg commented on May 31, 2017

   

   josephg

   on May 31, 2017

   Last edited by josephg

   Contributor

   More actions

   .... Er, yeah, sure is @vsemozhetbyt :)

   Actually the first method would consider these sets to be equal:

   new Set([{a:1}, {a:1}, {a:2}])
   new Set([{a:1}, {a:2}, {a:2}])

   So we should fix this using the second method.

   👍React with 👍1Reacted by Vse Mozhe Buty
9. josephg commented on Jun 3, 2017

   

   josephg

   on Jun 3, 2017

   Contributor

   More actions

   This also fails to throw, due to the same bug:

   assert.deepEqual(new Set([3, '3']), new Set([3, 4]));
   // and
   assert.deepEqual(new Map([[3, 0], ['3', 0]]), new Map([[3, 0], [4, 0]]);
10. refack

    added a commit that references this issue on Jun 5, 2017

    assert: fix deepEqual similar sets and maps bug

    ...

    7cddcc9
11. refack

    closed this as completedin 7cddcc9on Jun 5, 2017
12. jasnell

    added a commit that references this issue on Jun 7, 2017

    assert: fix deepEqual similar sets and maps bug

    ...

    b8e90dd
13. escKeyStroke commented on Oct 17, 2017

    

    escKeyStroke

    on Oct 17, 2017

    Last edited by escKeyStroke

    More actions

    A simpler case

    ... and heads up by a bro from lodash.js
    (version 6.11.4, Windows x64)

    assert.deepEqual( new Map([['x', 'y']]),
                      new Map([['y', 'x']]) ); // Doesn't throw

    I assert that this is fixed in v8.7.0.
14. josephg commented on Oct 17, 2017

    

    josephg

    on Oct 17, 2017

    Contributor

    More actions

    Primitive support for map and set equality wasn't added until node 8. In node 6 all maps and all sets are considered equivalent. These also didn't throw in node 7 and below:

    assert.deepEqual(new Map(), new Map())
    assert.deepEqual(new Map(), new Set())

    👍React with 👍1Reacted by EscKeyStroke