New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
timoxley opened this Issue Aug 5, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@timoxley
Copy link
Contributor

timoxley commented Aug 5, 2015

Currently assert.deepEquals is ignorant of any new native data types introduced in ES6.

For example, this should probably throw, yet it does not:

assert.deepEqual(new Set([1, 2]), new Set([1]))

A good method might be to deepEqual against .keys() and .values().
deepEqual should also probably work with generic Iterables too.

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Aug 5, 2015

I guess a major issue is that there are many interpretations of what "deep equals" even means and perhaps making something sensible for ES6 data types would be inconsistent with the existing algorithm.

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Aug 5, 2015

assert treats objects as key-value pairs. I don't think we should change that. This is something that can be done in a million different ways so it's probably better to leave that to user land. Also using it in a test is usually a bad idea, since for maintaining backwards compatibility you should check that specific properties exist and no that nothing else doesn't exist.

One interesting idea that requires some sort of standardisation is symbol properties that provide custom identity and equality functions (like Java equals and hashCode), but it's up to TC39.

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Aug 5, 2015

@vkurchatkin

assert treats objects as key-value pairs

Not all objects though, native Object types that existed in ES5 are special cased:
https://github.com/nodejs/io.js/blob/2a7fd0ad328d5197a9a166650e9eaa51367e3152/lib/assert.js#L152-L165

Set and Map are no more or less special than Date or RegExp.

Also using it in a test is usually a bad idea

Note that the tests for Node itself uses deepEqual extensively.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Aug 6, 2015

I agree that it probably should be updated to properly deeply evaluate into ES6+ data types.

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Aug 6, 2015

Not all objects though, native Object types that existed in ES5 are special cased

That is true, my bad. But these are not container types.

Note that the tests for Node itself uses deepEqual extensively

They use it to compare maps and lists of maps mostly. Otherwise it's possible to get unexpected results

@timoxley

This comment has been minimized.

Copy link
Contributor

timoxley commented Sep 16, 2015

Copying in @domenic's comment regarding this from #2315

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.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 19, 2015

The TSC decided this week to lock the assert API and there's a pull request that will likely land in the next several hours to update the documentation to that effect and to suggest that people use userland assertion libraries. Given that, I think it's appropriate to close this issue, but if anyone feels differently, by all means, re-open.

@josephg

This comment has been minimized.

Copy link
Contributor

josephg commented Mar 29, 2017

This is disappointing. I enjoy the simplicity of using the built-in assert library, and the semantics for what deep equal means are well defined for sets and maps (a.size === b.size && everything in a is in b). And I don't see how this change would break backwards compatibility - the behaviour of assert in the face of ES6 types was always undefined.

Time to go trawling through npm.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 29, 2017

@josephg The info in this issue is out of date. Subsequent to this decision, the locking of the API was reversed. (In fact, no APIs are locked anymore.) Any common-sense improvements to behavior of existing assert methods (such as assert.deepEqual()) are likely to be accepted, IMO.

@josephg

This comment has been minimized.

Copy link
Contributor

josephg commented Mar 29, 2017

Great, yeah I just read substack/node-deep-equal#28 . If nobody else has done it I'll put together a PR to fix this behaviour.

josephg added a commit to josephg/node that referenced this issue Mar 31, 2017

assert: Add support for Map and Set in deepEqual
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: substack/tape#342
Refs: nodejs#2315
Refs: nodejs/CTC#63

addaleax added a commit that referenced this issue Apr 3, 2017

assert: add support for Map and Set in deepEqual
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: substack/tape#342
Refs: #2315
Refs: nodejs/CTC#63
PR-URL: #12142
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment