Skip to content
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: Add support for Map and Set in deepEqual #12142

Closed
wants to merge 13 commits into from
98 changes: 97 additions & 1 deletion lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
// UTILITY
const compare = process.binding('buffer').compare;
const util = require('util');
const { isSet, isMap } = process.binding('util');
const objectToString = require('internal/util').objectToString;
const Buffer = require('buffer').Buffer;

Expand Down Expand Up @@ -262,11 +263,12 @@ function _deepEqual(actual, expected, strict, memos) {
}
}

// For all other Object pairs, including Array objects,
// For all other Object pairs, including Array objects and Maps,
// equivalence is determined by having:
// a) The same number of owned enumerable properties
// b) The same set of keys/indexes (although not necessarily the same order)
// c) Equivalent values for every corresponding key/index
// d) For Maps, strict-equal keys mapping to deep-equal values
// Note: this accounts for both named and indexed properties on Arrays.

// Use memos to handle cycles.
Expand All @@ -280,9 +282,87 @@ function _deepEqual(actual, expected, strict, memos) {
memos.actual.push(actual);
memos.expected.push(expected);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed..unrelated white space change?

return objEquiv(actual, expected, strict, memos);
}

function setEquiv(a, b, strict, actualVisitedObjects) {
// This code currently returns false for this pair of sets:
// assert.deepEqual(new Set(['1', 1]), new Set([1]))
//
// In theory, all the items in the first set have a corresponding == value in
// the second set, but the sets have different sizes. Should they be
// considered to be non-strict deep equal to one another? Its a silly case,
// and more evidence that deepStrictEqual should always be preferred over
// deepEqual. The implementation currently returns false, which is a simpler
// and faster implementation.
if (a.size !== b.size)
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I think deepEqual should be correct to reject sets of different sizes.


var val1, val2;
outer: for (val1 of a) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here?

if (!b.has(val1)) {
// The value doesn't exist in the second set by reference, so we'll go
// hunting for something thats deep-equal to it. Note that this is O(n^2)
// complexity, and will get slower if large, very similar sets / maps are
// nested inside. Unfortunately there's no real way around this.
Copy link
Member

@TimothyGu TimothyGu Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to optimize this by only starting the additional search if !strict || typeof val1 === 'object'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thats really clever, and very obvious now that you've pointed it out. Fixed.

for (val2 of b) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const here?

if (_deepEqual(val1, val2, strict, actualVisitedObjects)) {
continue outer;
}
}

// Not found!
return false;
}
}

return true;
}

function mapEquiv(a, b, strict, actualVisitedObjects) {
// Caveat: In non-strict mode, this implementation does not handle cases
// where maps contain two equivalent-but-not-reference-equal keys.
//
// For example, maps like this are currently considered not equivalent:
if (a.size !== b.size)
return false;

var key1, key2, item1, item2;
outer: for ([key1, item1] of a) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use const here as well?

// To be able to handle cases like:
// Map([[1, 'a'], ['1', 'b']]) vs Map([['1', 'a'], [1, 'b']])
// or:
// Map([[{}, 'a'], [{}, 'b']]) vs Map([[{}, 'b'], [{}, 'a']])
// ... we need to consider *all* matching keys, not just the first we find.

// This check is not strictly necessary, but its here to improve
// performance of the common case when reference-equal keys exist (which
// includes all primitive valued keys).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: primitive-valued

if (b.has(key1)) {
if (_deepEqual(item1, b.get(key1), strict, actualVisitedObjects))
continue outer;
}

// Hunt for keys which are deep-equal to key1 in b. Just like setEquiv
// above, this hunt makes this function O(n^2).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

for ([key2, item2] of b) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you use const here as well?

// Just for performance. We already checked these keys above.
if (key2 === key1)
continue;

if (_deepEqual(key1, key2, strict, actualVisitedObjects) &&
_deepEqual(item1, item2, strict, actualVisitedObjects)) {
continue outer;
}
}

return false;
}

return true;
}

function objEquiv(a, b, strict, actualVisitedObjects) {
// If one of them is a primitive, the other must be the same.
if (util.isPrimitive(a) || util.isPrimitive(b))
Expand All @@ -307,6 +387,22 @@ function objEquiv(a, b, strict, actualVisitedObjects) {
return false;
}

// Sets and maps don't have their entries accessable via normal object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/accessable/accessible/

// properties.
if (isSet(a)) {
if (!isSet(b) || !setEquiv(a, b, strict, actualVisitedObjects))
return false;
} else if (isSet(b)) {
return false;
}

if (isMap(a)) {
if (!isMap(b) || !mapEquiv(a, b, strict, actualVisitedObjects))
return false;
} else if (isMap(b)) {
return false;
}

// The pair must have equivalent values for every corresponding key.
// Possibly expensive deep test:
for (i = aKeys.length - 1; i >= 0; i--) {
Expand Down
140 changes: 139 additions & 1 deletion test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,11 +100,149 @@ const similar = new Set([
for (const a of similar) {
for (const b of similar) {
if (a !== b) {
assert.doesNotThrow(() => assert.deepEqual(a, b));
assert.deepEqual(a, b);
assert.throws(() => assert.deepStrictEqual(a, b),
re`${a} deepStrictEqual ${b}`);
}
}
}

function assertDeepAndStrictEqual(a, b) {
assert.doesNotThrow(() => assert.deepEqual(a, b));
assert.doesNotThrow(() => assert.deepStrictEqual(a, b));

assert.doesNotThrow(() => assert.deepEqual(b, a));
assert.doesNotThrow(() => assert.deepStrictEqual(b, a));
}

function assertNotDeepOrStrict(a, b) {
assert.throws(() => assert.deepEqual(a, b));
assert.throws(() => assert.deepStrictEqual(a, b));

assert.throws(() => assert.deepEqual(b, a));
assert.throws(() => assert.deepStrictEqual(b, a));
}

function assertOnlyDeepEqual(a, b) {
assert.doesNotThrow(() => assert.deepEqual(a, b));
assert.throws(() => assert.deepStrictEqual(a, b));

assert.doesNotThrow(() => assert.deepEqual(b, a));
assert.throws(() => assert.deepStrictEqual(b, a));
}

// es6 Maps and Sets
assertDeepAndStrictEqual(new Set(), new Set());
assertDeepAndStrictEqual(new Map(), new Map());

assertDeepAndStrictEqual(new Set([1, 2, 3]), new Set([1, 2, 3]));
assertNotDeepOrStrict(new Set([1, 2, 3]), new Set([1, 2, 3, 4]));
assertNotDeepOrStrict(new Set([1, 2, 3, 4]), new Set([1, 2, 3]));
assertDeepAndStrictEqual(new Set(['1', '2', '3']), new Set(['1', '2', '3']));
assertDeepAndStrictEqual(new Set([[1, 2], [3, 4]]), new Set([[3, 4], [1, 2]]));

assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[1, 1], [2, 2]]));
assertDeepAndStrictEqual(new Map([[1, 1], [2, 2]]), new Map([[2, 2], [1, 1]]));
assertNotDeepOrStrict(new Map([[1, 1], [2, 2]]), new Map([[1, 2], [2, 1]]));

assertNotDeepOrStrict(new Set([1]), [1]);
assertNotDeepOrStrict(new Set(), []);
assertNotDeepOrStrict(new Set(), {});

assertNotDeepOrStrict(new Map([['a', 1]]), {a: 1});
assertNotDeepOrStrict(new Map(), []);
assertNotDeepOrStrict(new Map(), {});

assertOnlyDeepEqual(new Set(['1']), new Set([1]));

assertOnlyDeepEqual(new Map([['1', 'a']]), new Map([[1, 'a']]));
assertOnlyDeepEqual(new Map([['a', '1']]), new Map([['a', 1]]));

// This is an awful case, where a map contains multiple equivalent keys:
assertOnlyDeepEqual(
new Map([[1, 'a'], ['1', 'b']]),
new Map([['1', 'a'], [1, 'b']])
);
assertDeepAndStrictEqual(
new Map([[{}, 'a'], [{}, 'b']]),
new Map([[{}, 'b'], [{}, 'a']])
);

{
const values = [
123,
Infinity,
0,
null,
undefined,
false,
true,
{},
[],
() => {},
];
assertDeepAndStrictEqual(new Set(values), new Set(values));
assertDeepAndStrictEqual(new Set(values), new Set(values.reverse()));

const mapValues = values.map((v) => [v, {a: 5}]);
assertDeepAndStrictEqual(new Map(mapValues), new Map(mapValues));
assertDeepAndStrictEqual(new Map(mapValues), new Map(mapValues.reverse()));
}

{
const s1 = new Set();
const s2 = new Set();
s1.add(1);
s1.add(2);
s2.add(2);
s2.add(1);
assertDeepAndStrictEqual(s1, s2);
}

{
const m1 = new Map();
const m2 = new Map();
const obj = {a: 5, b: 6};
m1.set(1, obj);
m1.set(2, 'hi');
m1.set(3, [1, 2, 3]);

m2.set(2, 'hi'); // different order
m2.set(1, obj);
m2.set(3, [1, 2, 3]); // deep equal, but not reference equal.

assertDeepAndStrictEqual(m1, m2);
}

{
const m1 = new Map();
const m2 = new Map();

// m1 contains itself.
m1.set(1, m1);
m2.set(1, new Map());

assertNotDeepOrStrict(m1, m2);
}

assert.deepEqual(new Map([[1, 1]]), new Map([[1, '1']]));
assert.throws(() =>
assert.deepStrictEqual(new Map([[1, 1]]), new Map([[1, '1']]))
);

{
// Two equivalent sets / maps with different key/values applied shouldn't be
// the same. This is a terrible idea to do in practice, but deepEqual should
// still check for it.
const s1 = new Set();
const s2 = new Set();
s1.x = 5;
assertNotDeepOrStrict(s1, s2);

const m1 = new Map();
const m2 = new Map();
m1.x = 5;
assertNotDeepOrStrict(m1, m2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a case where there is a circular reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


/* eslint-enable */