Comparisons don't work as expected with the ES6 / ES2015 Set object

[trevordmiller]

Issue

I'm using the expect package from npm, which uses this package under the hood to check for deep equality. In my eyes, the test below should fail (different values and different references), but it is passing:

it('should fail', () => {
  const actual = new Set('a');
  const expected = new Set('b');
  expect(actual).toEqual(expected);
});

If I did something like this with another collection (like arrays), the test wouldn't pass until 1) the references are the same with assert.equal / , expect.toBe or 2) the values are the same with assert.deepEqual / expect.toEqual

Background

I'm using Babel 5 to use Set. I'm using Node 5, so the Set being used should be native.
Am I doing something wrong, or does this look like a bug in the expect package in the way it handles Sets? I've cross posted a Stack Overflow question as I'm not sure.

I originally posted this on the expect package issues, but they sent me here, since that package uses this package ;)
mjackson/expect#47 (comment)

cc: @mjackson, @rstacruz

It seems that node core assert.deepEqual has the same bug. Here's how it behaves on node 5.1.0:

$ node -e "var a = new Set('a'), b = new Set('x'); require('assert').deepEqual(a, b)"
$ echo $?
0

Relevant node core discussion: nodejs/node#2309

This is a bit of a tricky decision, since node-deep-equal is meant to implement node's assert.deepEqual() algorithm, but core has decided to lock its API and push handling new native data types in userland.

[mjackson]

I guess it's ultimately up to you, @substack, whether you want to follow their lead or not. @domenic suggested that node's assert module should only be used to test node, so it should be left as-is in core, and I agree. Until there's demand for it in core there's no need to include it there. It's not meant to be a general purpose assertion module.

In this package, however, I think you do have a little more flexibility.

For starters, in the assert package they currently follow core's lead as well so anyone who wants node core behavior can just use require('assert').deepEqual and it will work the same everywhere. If people show up here and complain that deepEqual doesn't work the same as in node, you can just point them to the assert package and tell them to use that if they want identical behavior.

Additionally, including support for more native data types here is an enhancement, so it won't break existing code.

If you rename the repo from node-deep-equal to just deep-equal (same as the package) then it should convey more clearly to users that this is a general purpose equality comparison, not just for node.

In any case, ultimately we'll need a deepEqual library that knows about additional native types as they are added. They'll need it in core too, eventually.

Just a few thoughts. Hope it helps.

[domenic]

I'm not really sure in general that deepEqual is the right name for something that compares things based on their private state. You'd want something like "structurallyEqual" or "privateEqual" I think. Map, Set, Date, and Promise are all types which you could invent equality semantics for based on examining whether they have the same internal data, but "deep equal" traditionally means "has the same public properties" which will be true of any two Maps etc.

[trevordmiller]

To sum things up: I need a way to write test assertions to compare the values in two ES2015 Sets. I really like @mjackson's expect package, but I can use another package or build a helper comparison function if @substack doesn't want to support this; it just seems weird to me that there is no simple way to compare Sets, which are now native JavaScript objects 😜

cc: @statianzo

[mjackson]

Update: We don't have this issue anymore in the expect package since we switched to https://github.com/ljharb/is-equal. Thanks for your time :)

[trevordmiller]

@mjackson So I can compare Sets in the same way I compare arrays? Awesome! Thank you.

[svachalek]

I encountered this problem using tape today. While the node team decided to take their ball and go home, it seems that tape (and this package) are left on the lonely playground they call "userland".

It's probably small-minded of me but I can't see a good reason a test assertion would want to find all Set objects "equal", and it sure would be nice if this library did the Right Thing rather than imitate its namesake's failings.

[mjackson]

Scott, grow up. Your tone here is ridiculous.
On Fri, May 20, 2016 at 4:40 PM Scott Vachalek notifications@github.com
wrote:

I encountered this problem using tape today. While the node team decided
to take their ball and go home, it seems that tape (and this package) are
left on the lonely playground they call "userland".

It's probably small-minded of me but I can't see a good reason a test
assertion would want to find all Set objects "equal", and it sure would be
nice if this library did the Right Thing rather than imitate its namesake's
failings.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAFqpzmVoAAzaCbwKDZ-BrlH94g5nqWrks5qDkZPgaJpZM4GpY59
.

[svachalek]

My tone is light and playful, but sincere. I can switch testing and/or assertion libraries but before I do that, I would like to add a vote on the side of usefulness over semantics. If Set seems like too much of a special case, consider solving it for the iterable protocol.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#iterable

[SeeThruHead]

Is there anything in the works for this? I'm doing deepEqual on something that contains a map, having to run my expected and result objects through something that unwraps a map into it's array version is rather cumbersome. Maybe a separate assertion that uses isEqual?