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
label
Mar 31, 2017
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. Ref: nodejs#12142
Based on comments in the PR, this change restricts an O(n^2) to only happen when your set contains object-like objects, your map contains object-like keys or you're not in strict mode. ref: nodejs#12142 (review)
and removed semver-minor
PRs that contain new features and should be released in the next minor version.labels
Mar 31, 2017
- Added changes: entries in assert API documentation - Refactored setEquiv based on @joyeecheung's comments ref: nodejs#12142
Refactored setEquiv and mapEquiv based on @joyeecheung's stylistic preference to avoid labels.
addaleax pushed a commit that referenced this pull request
Apr 3, 2017
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 <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>
Closed
Closed
4 tasks
Closed
josephg deleted the assert-sets-and-maps branch
June 2, 2017 23:53josephg added a commit to josephg/node that referenced this pull request
Jun 3, 2017
This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. Fixes: nodejs#13347 Refs: nodejs#12142
Closed
3 tasks
refack pushed a commit to refack/node that referenced this pull request
Jun 5, 2017
This fixes a bug where deepEqual and deepStrictEqual would have incorrect behaviour in sets and maps containing multiple equivalent keys. PR-URL: nodejs#13426 Fixes: nodejs#13347 Refs: nodejs#12142 Reviewed-By: Refael Ackermann <refack@gmail.com>
jasnell pushed a commit that referenced this pull request
Jun 7, 2017