assert: add support for Set and Map #2315

evanlucas · 2015-08-06T20:20:43Z

Previously, assert.deepEqual used on maps and sets was not functioning
correctly.

This change adds support for correctly comparing Maps and Sets.

For Maps, comparison is done by iterating over each item in both Maps
and verifying that the key exists and the value is the same (depending
on whether strict is applied).

For Sets, each item is iterated in each Set and checked if it exists in
the other Set.

For both Maps and Sets, the size of both being compared is checked as
well.

Related: #2309

Fishrock123 · 2015-08-06T21:28:08Z

lib/assert.js

+
+function compareSetItems(a, b, strict) {
+  for (let item of a) {
+    if (!b.has(item))


It should check the same index for sets, not sure about maps.

Index? You mean the order?

Strictly speaking sets are unordered, should we really worry about it?

I think I agree with @Fishrock123 here. Order should probably be checked as well IMO.

Hmm, sets do seem to be kind of unordered, you can't just "get" an index: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set

Strictly speaking sets are unordered

Strictly correct, but note that unlike ES5 Objects where iteration is in "whatever, probably insertion" order, iteration over Sets and Maps is defined in ES2015 to occur in "insertion order". Despite this ordered iteration, one could consider the underlying data represented by the Set/Map datastructure to be unordered though.

While verifying order may be more strictly "equal", I disagree that this is very useful in practice though. Ignoring the spec, consider whether deepEqual would be more or less useful if it also verified the key order for objects. IMO, for common use cases, verification of the iteration order is not important. If you do want to additionally verify the ordering, it's trivial enough to do: convert the Set/Map.keys() to arrays and deepEqual the arrays.

Consider then if deepEqual does check order and one wants to ignore the iteration order one will need to create an array of the keys/values, sort it somehow (depends on data), then use this to create a new Map/Set. Checking iteration order would make the IMO common usecase more verbose.

It should check the same index for sets, not sure about maps.

deepEqual over Sets and Maps should exhibit the same behaviour with regard to order.

Sounds good to me.

targos · 2015-08-07T06:17:18Z

lib/assert.js

+  if (!compareMapKeys(a, b, strict))
+    return false;
+
+  return compareMapKeys(b, a, strict);


Knowing that we check size equality before, do we need to iterate on each Map ?

Good point.

evanlucas · 2015-08-07T12:41:24Z

Comments have been addressed and additional tests have been added to verify that order does not matter.

evanlucas · 2015-08-07T12:44:04Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/229/

targos · 2015-08-07T12:55:34Z

LGTM

evanlucas · 2015-08-07T13:27:20Z

In addition to master, should this also be landed in 3.x?

thefourtheye · 2015-08-07T13:38:11Z

function compareSet(a, b, strict) { strict is unused

evanlucas · 2015-08-07T13:40:25Z

@thefourtheye good catch. Removed

thefourtheye · 2015-08-07T13:47:21Z

// throws because we can't really check without being strict I think this comment can be done better. For sets, the importance of strict is only with the prototype comparison. It shouldnt matter for the values comparison. Probably I am missing something.

PS. I am in a forest with a bad internet connection. Sorry for leaving comments with a lot of delay :'(

thefourtheye · 2015-08-07T13:50:51Z

Can we use const wherever the values are assigned only once?

evanlucas · 2015-08-07T13:51:03Z

If we are comparing strictly, it is equivalent to using a === b. With sets, all we can do is see if set.has(a). We can't apply type coercion in that to my knowledge

evanlucas · 2015-08-07T13:52:58Z

Updated to use const

thefourtheye · 2015-08-07T13:53:20Z

Exactly. It doesn't matter if we are strict or not in this case, right?

evanlucas · 2015-08-07T13:54:30Z

Yes, it will be "strict" in the sense that it checks strict equality regardless

evanlucas · 2015-08-07T13:56:26Z

Unless, we were to use _deepEqual(a, b, strict) on each value.

Edit: which may be necessary.

Ex:

var set1 = new Set([ { name: 'test' } ])
var set2 = new Set([ { name: 'test' } ])
// throws
assert.deepEqual(set1, set2)

var obj = { name: 'test' }
var set1 = new Set([obj])
var set2 = new Set([obj])
// does not throw
assert.deepEqual(set1, set2)

thefourtheye · 2015-08-07T14:00:59Z

Yes. Okay, if that comment is clear to others, I am okay. One minor nit, you can even ignore it, I feel that compare{Maps, Sets} would be better.

thefourtheye · 2015-08-07T14:03:39Z

Nah, I don't think deepEqual is necessary as of now. What would be interesting is, when the custom comparison feature lands in the language.

Previously, assert.deepEqual used on maps and sets was not functioning correctly. This change adds support for correctly comparing Maps and Sets. For Maps, comparison is done by iterating over each item in the first Map and verifying that the key exists and the value is the same (depending on whether strict is applied). For Sets, each item is iterated over in the first Set and checked if it exists in the other Set. For both Maps and Sets, the size of both being compared is checked as well.

evanlucas · 2015-08-07T14:12:27Z

Now that I think about it though, should strict for sets not compare each item of sets using deepStrictEqual?

Edit: I guess that would go back to ordering being strict, so nevermind

evanlucas · 2015-08-07T14:20:39Z

CI looks happy.

thefourtheye · 2015-08-07T14:20:56Z

Then we will be contradicting the result of Set.has. IIRC, it uses strict equal comparison internally. If we use deepStrictEqual it might return true for similar objects but has will return false.

targos · 2015-08-07T14:21:15Z

Is it semver-minor ?

evanlucas · 2015-08-07T14:22:26Z

Honestly, I would think this would be considered a bug fix?

thefourtheye · 2015-08-07T14:23:41Z

+1 for semver-minor

targos · 2015-08-07T14:28:32Z

One could also say it is semver-major because the behaviour changes for:

var a = new Set([1, 2, 3]);
a.x = 1;
var b = {x: 1};
assert.deepEqual(a, b);

I also see it as a bug fix btw.

thefourtheye · 2015-08-07T14:34:40Z

I need to check a few things, but bug fix sounds good as targos points out. We should also note that it is not a backward compatible change. Ah its confusing me. Sorry, I am not sure :-(

yosuke-furukawa · 2015-08-07T17:18:24Z

lib/assert.js

+    return false;
+
+  // iterate over all keys and check if they exist in the other
+  for (let kvs of a) {


let is not faster than var, if this code is hotcode, we would be better to use var instead of let.

Good point. I wouldn't consider this to be hot code, but probably should stick with var for now?

it is not faster, but performs equally in this kind of situation: http://jsperf.com/let-vs-var-iteration
I believe the optimization arrived with V8 4.4.

domenic · 2015-08-11T20:41:53Z

I don't think we should do this (or anything else in #2309). Assert should remain used only for testing io.js itself, and not try to be a good general assertion library. If io.js tests need this ability enough times that we need to factor it out, we should, but until then, just adding it because it'd be nice isn't a good idea IMO.

evanlucas · 2015-08-12T12:40:39Z

@domenic fair enough. This could be done in user-land fairly easy

evanlucas · 2015-08-12T13:49:49Z

I went ahead and published an npm package that supports this.

https://github.com/evanlucas/assert6

sam-github · 2017-01-24T15:39:59Z

Now that maps and sets are in v8, its possible for node itself to start using them internally, in which case assert's deepEqual won't work for node itself. I guess we could wait until this actually happens, and some node contributor gets terribly confused, or fix it now.

assert.deepEqual and assert.deepStrictEqual currently return true for any pair of Maps and Sets regardless of content. This patch adds support in deepEqual and deepStrictEqual to verify the contents of Maps and Sets. Unfortunately because there's no way to pairwise fetch set values or map values which are equivalent but not reference-equal, this change currently only supports reference equality checking in set values and map key values. Equivalence checking could be done, but it would be an O(n^2) operation, and worse, it would get slower exponentially if maps and sets were nested. Note that this change breaks compatibility with previous versions of deepEqual and deepStrictEqual if consumers were depending on all maps and sets to be seen as equivalent. The old behaviour was never documented, but nevertheless there are certainly some tests out there which depend on it. Support has stalled because the assert API was frozen, but was recently unfrozen in CTC#63 Fixes: nodejs#2309 Refs: tape-testing/tape#342 Refs: nodejs#2315 Refs: nodejs/CTC#63

assert.deepEqual and assert.deepStrictEqual currently return true for any pair of Maps and Sets regardless of content. This patch adds support in deepEqual and deepStrictEqual to verify the contents of Maps and Sets. Deeo equivalence checking is currently an O(n^2) operation, and worse, it gets slower exponentially if maps and sets were nested. Note that this change breaks compatibility with previous versions of deepEqual and deepStrictEqual if consumers were depending on all maps and sets to be seen as equivalent. The old behaviour was never documented, but nevertheless there are certainly some tests out there which depend on it. Support has stalled because the assert API was frozen, but was recently unfrozen in CTC#63. --- Later squashed in: This change updates the checks for deep equality checking on Map and Set to check all set values / all map keys to see if any of them match the expected result. This change is much slower, but based on the conversation in the pull request its probably the right approach. Fixes: #2309 Refs: tape-testing/tape#342 Refs: #2315 Refs: nodejs/CTC#63 PR-URL: #12142 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

evanlucas added the assert Issues and PRs related to the assert subsystem. label Aug 6, 2015

Fishrock123 reviewed Aug 6, 2015
View reviewed changes

evanlucas force-pushed the assertmapset branch from 3071b08 to bc3ca40 Compare August 7, 2015 00:34

targos reviewed Aug 7, 2015
View reviewed changes

evanlucas force-pushed the assertmapset branch from bc3ca40 to 51816c3 Compare August 7, 2015 12:39

evanlucas force-pushed the assertmapset branch from 51816c3 to b2a1ae5 Compare August 7, 2015 13:40

evanlucas force-pushed the assertmapset branch from b2a1ae5 to d963144 Compare August 7, 2015 13:52

evanlucas force-pushed the assertmapset branch from d963144 to 68b723d Compare August 7, 2015 14:11

yosuke-furukawa reviewed Aug 7, 2015
View reviewed changes

evanlucas closed this Aug 21, 2015

timoxley mentioned this pull request Sep 16, 2015

assert.deepEqual & post-ES5 Data Types e.g. Map, Set, Iterables, etc #2309

Closed

thefourtheye mentioned this pull request Sep 30, 2015

lib: generalize handling in assert.deepEqual() #3124

Closed

sindresorhus mentioned this pull request Oct 31, 2015

Match core assert API avajs/ava#114

Closed

thefourtheye mentioned this pull request Apr 29, 2016

assert: allow circular references #6432

Closed

3 tasks

evanlucas mentioned this pull request Jan 24, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-01-25 nodejs/CTC#63

Closed

Trott mentioned this pull request Feb 6, 2017

Support for comparing ES2015 Maps and Sets tape-testing/tape#342

Closed

josephg mentioned this pull request Mar 31, 2017

assert: Add support for Map and Set in deepEqual #12142

Closed

3 tasks

assert: add support for Set and Map #2315

assert: add support for Set and Map #2315

Conversation

evanlucas commented Aug 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanlucas commented Aug 7, 2015

evanlucas commented Aug 7, 2015

targos commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

evanlucas commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

evanlucas commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

evanlucas commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

targos commented Aug 7, 2015

evanlucas commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

targos commented Aug 7, 2015

thefourtheye commented Aug 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic commented Aug 11, 2015

evanlucas commented Aug 12, 2015

evanlucas commented Aug 12, 2015

sam-github commented Jan 24, 2017