Skip to content

Commit

Permalink
assert: handle enumerable symbol keys
Browse files Browse the repository at this point in the history 
PR-URL: #15169
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
  • Loading branch information
@BridgeAR
BridgeAR committed Sep 19, 2017
1 parent a8c0a43 commit db2e093
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 62 deletions.
25 changes: 18 additions & 7 deletions doc/api/assert.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,11 @@ Primitive values are compared with the [Abstract Equality Comparison][]

Only [enumerable "own" properties][] are considered. The
[`assert.deepEqual()`][] implementation does not test the
[`[[Prototype]]`][prototype-spec] of objects, attached symbols, or
non-enumerable properties — for such checks, consider using
[`assert.deepStrictEqual()`][] instead. This can lead to some
potentially surprising results. For example, the following example does not
throw an `AssertionError` because the properties on the [`RegExp`][] object are
not enumerable:
[`[[Prototype]]`][prototype-spec] of objects or enumerable own [`Symbol`][]
properties. For such checks, consider using [assert.deepStrictEqual()][]
instead. [`assert.deepEqual()`][] can have potentially surprising results. The
following example does not throw an `AssertionError` because the properties on
the [RegExp][] object are not enumerable:

```js
// WARNING: This does not throw an AssertionError!
Expand Down Expand Up @@ -109,6 +108,9 @@ parameter is an instance of an `Error` then it will be thrown instead of the
<!-- YAML
added: v1.2.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15169
description: Enumerable symbol properties are now compared.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/15036
description: NaN is now compared using the [SameValueZero][] comparison.
Expand All @@ -132,7 +134,7 @@ changes:
* `expected` {any}
* `message` {any}

Generally identical to `assert.deepEqual()` with a few exceptions:
Similar to `assert.deepEqual()` with the following exceptions:

1. Primitive values besides `NaN` are compared using the [Strict Equality
Comparison][] ( `===` ). Set and Map values, Map keys and `NaN` are compared
Expand All @@ -143,6 +145,7 @@ Generally identical to `assert.deepEqual()` with a few exceptions:
3. [Type tags][Object.prototype.toString()] of objects should be the same.
4. [Object wrappers][] are compared both as objects and unwrapped values.
5. `0` and `-0` are not considered equal.
6. Enumerable own [`Symbol`][] properties are compared as well.

```js
const assert = require('assert');
Expand Down Expand Up @@ -185,6 +188,13 @@ assert.deepStrictEqual(-0, -0);
// OK
assert.deepStrictEqual(0, -0);
// AssertionError: 0 deepStrictEqual -0

const symbol1 = Symbol();
const symbol2 = Symbol();
assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol1]: 1 });
// OK, because it is the same symbol on both objects.
assert.deepStrictEqual({ [symbol1]: 1 }, { [symbol2]: 1 });
// Fails because symbol1 !== symbol2!
```

If the values are not equal, an `AssertionError` is thrown with a `message`
Expand Down Expand Up @@ -712,6 +722,7 @@ For more information, see
[`Object.is()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
[`RegExp`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions
[`Set`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Set
[`Symbol`]: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol
[`TypeError`]: errors.html#errors_class_typeerror
[`assert.deepEqual()`]: #assert_assert_deepequal_actual_expected_message
[`assert.deepStrictEqual()`]: #assert_assert_deepstrictequal_actual_expected_message
Expand Down
125 changes: 74 additions & 51 deletions lib/assert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const { compare } = process.binding('buffer');
const { isSet, isMap, isDate, isRegExp } = process.binding('util');
const { objectToString } = require('internal/util');
const errors = require('internal/errors');
const { propertyIsEnumerable } = Object.prototype;

// The assert module provides functions that throw
// AssertionError's when particular conditions are not met. The
Expand Down Expand Up @@ -165,7 +166,7 @@ function isObjectOrArrayTag(tag) {
// For strict comparison, objects should have
// a) The same built-in type tags
// b) The same prototypes.
function strictDeepEqual(actual, expected) {
function strictDeepEqual(actual, expected, memos) {
if (typeof actual !== 'object') {
return typeof actual === 'number' && Number.isNaN(actual) &&
Number.isNaN(expected);
Expand All @@ -186,12 +187,12 @@ function strictDeepEqual(actual, expected) {
// Check for sparse arrays and general fast path
if (actual.length !== expected.length)
return false;
// Skip testing the part below and continue in the callee function.
return;
// Skip testing the part below and continue with the keyCheck.
return keyCheck(actual, expected, true, memos);
}
if (actualTag === '[object Object]') {
// Skip testing the part below and continue in the callee function.
return;
// Skip testing the part below and continue with the keyCheck.
return keyCheck(actual, expected, true, memos);
}
if (isDate(actual)) {
if (actual.getTime() !== expected.getTime()) {
Expand All @@ -215,10 +216,8 @@ function strictDeepEqual(actual, expected) {
}
// Buffer.compare returns true, so actual.length === expected.length
// if they both only contain numeric keys, we don't need to exam further
if (Object.keys(actual).length === actual.length &&
Object.keys(expected).length === expected.length) {
return true;
}
return keyCheck(actual, expected, true, memos, actual.length,
expected.length);
} else if (typeof actual.valueOf === 'function') {
const actualValue = actual.valueOf();
// Note: Boxed string keys are going to be compared again by Object.keys
Expand All @@ -232,15 +231,14 @@ function strictDeepEqual(actual, expected) {
lengthActual = actual.length;
lengthExpected = expected.length;
}
if (Object.keys(actual).length === lengthActual &&
Object.keys(expected).length === lengthExpected) {
return true;
}
return keyCheck(actual, expected, true, memos, lengthActual,
lengthExpected);
}
}
return keyCheck(actual, expected, true, memos);
}

function looseDeepEqual(actual, expected) {
function looseDeepEqual(actual, expected, memos) {
if (actual === null || typeof actual !== 'object') {
if (expected === null || typeof expected !== 'object') {
// eslint-disable-next-line eqeqeq
Expand Down Expand Up @@ -274,33 +272,62 @@ function looseDeepEqual(actual, expected) {
} else if (isArguments(actualTag) || isArguments(expectedTag)) {
return false;
}
return keyCheck(actual, expected, false, memos);
}

function innerDeepEqual(actual, expected, strict, memos) {
// All identical values are equivalent, as determined by ===.
if (actual === expected) {
if (actual !== 0)
return true;
return strict ? Object.is(actual, expected) : true;
}

// Returns a boolean if (not) equal and undefined in case we have to check
// further.
const partialCheck = strict ?
strictDeepEqual(actual, expected) :
looseDeepEqual(actual, expected);

if (partialCheck !== undefined) {
return partialCheck;
}

function keyCheck(actual, expected, strict, memos, lengthA, lengthB) {
// For all remaining 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 Sets and Maps, equal contents
// Note: this accounts for both named and indexed properties on Arrays.
var aKeys = Object.keys(actual);
var bKeys = Object.keys(expected);
var i;

// The pair must have the same number of owned properties.
if (aKeys.length !== bKeys.length)
return false;

if (strict) {
var symbolKeysA = Object.getOwnPropertySymbols(actual);
var symbolKeysB = Object.getOwnPropertySymbols(expected);
if (symbolKeysA.length !== 0) {
symbolKeysA = symbolKeysA.filter((k) =>
propertyIsEnumerable.call(actual, k));
symbolKeysB = symbolKeysB.filter((k) =>
propertyIsEnumerable.call(expected, k));
if (symbolKeysA.length !== symbolKeysB.length)
return false;
} else if (symbolKeysB.length !== 0 && symbolKeysB.filter((k) =>
propertyIsEnumerable.call(expected, k)).length !== 0) {
return false;
}
if (lengthA !== undefined) {
if (aKeys.length !== lengthA || bKeys.length !== lengthB)
return false;
if (symbolKeysA.length === 0)
return true;
aKeys = [];
bKeys = [];
}
if (symbolKeysA.length !== 0) {
aKeys.push(...symbolKeysA);
bKeys.push(...symbolKeysB);
}
}

// Cheap key test:
const keys = {};
for (i = 0; i < aKeys.length; i++) {
keys[aKeys[i]] = true;
}
for (i = 0; i < aKeys.length; i++) {
if (keys[bKeys[i]] === undefined)
return false;
}

// Use memos to handle cycles.
if (memos === undefined) {
Expand All @@ -323,25 +350,6 @@ function innerDeepEqual(actual, expected, strict, memos) {
memos.position++;
}

const aKeys = Object.keys(actual);
const bKeys = Object.keys(expected);
var i;

// The pair must have the same number of owned properties
// (keys incorporates hasOwnProperty).
if (aKeys.length !== bKeys.length)
return false;

// Cheap key test:
const keys = {};
for (i = 0; i < aKeys.length; i++) {
keys[aKeys[i]] = true;
}
for (i = 0; i < aKeys.length; i++) {
if (keys[bKeys[i]] === undefined)
return false;
}

memos.actual.set(actual, memos.position);
memos.expected.set(expected, memos.position);

Expand All @@ -353,6 +361,21 @@ function innerDeepEqual(actual, expected, strict, memos) {
return areEq;
}

function innerDeepEqual(actual, expected, strict, memos) {
// All identical values are equivalent, as determined by ===.
if (actual === expected) {
if (actual !== 0)
return true;
return strict ? Object.is(actual, expected) : true;
}

// Check more closely if actual and expected are equal.
if (strict === true)
return strictDeepEqual(actual, expected, memos);

return looseDeepEqual(actual, expected, memos);
}

function setHasEqualElement(set, val1, strict, memo) {
// Go looking.
for (const val2 of set) {
Expand Down
28 changes: 28 additions & 0 deletions test/parallel/test-assert-deep.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -507,8 +507,36 @@ assert.doesNotThrow(
boxedSymbol.slow = true;
assertNotDeepOrStrict(boxedSymbol, {});
}

// Minus zero
assertOnlyDeepEqual(0, -0);
assertDeepAndStrictEqual(-0, -0);

// Handle symbols (enumerable only)
{
const symbol1 = Symbol();
const obj1 = { [symbol1]: 1 };
const obj2 = { [symbol1]: 1 };
const obj3 = { [Symbol()]: 1 };
// Add a non enumerable symbol as well. It is going to be ignored!
Object.defineProperty(obj2, Symbol(), { value: 1 });
assertOnlyDeepEqual(obj1, obj3);
assertDeepAndStrictEqual(obj1, obj2);
// TypedArrays have a fast path. Test for this as well.
const a = new Uint8Array(4);
const b = new Uint8Array(4);
a[symbol1] = true;
b[symbol1] = false;
assertOnlyDeepEqual(a, b);
b[symbol1] = true;
assertDeepAndStrictEqual(a, b);
// The same as TypedArrays is valid for boxed primitives
const boxedStringA = new String('test');
const boxedStringB = new String('test');
boxedStringA[symbol1] = true;
assertOnlyDeepEqual(boxedStringA, boxedStringB);
boxedStringA[symbol1] = true;
assertDeepAndStrictEqual(a, b);
}

/* eslint-enable */
11 changes: 7 additions & 4 deletions test/parallel/test-net-normalize-args.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ function validateNormalizedArgs(input, output) {
}

// Test creation of normalized arguments.
validateNormalizedArgs([], [{}, null]);
validateNormalizedArgs([{ port: 1234 }], [{ port: 1234 }, null]);
validateNormalizedArgs([{ port: 1234 }, assert.fail],
[{ port: 1234 }, assert.fail]);
const res = [{}, null];
res[normalizedArgsSymbol] = true;
validateNormalizedArgs([], res);
res[0].port = 1234;
validateNormalizedArgs([{ port: 1234 }], res);
res[1] = assert.fail;
validateNormalizedArgs([{ port: 1234 }, assert.fail], res);

// Connecting to the server should fail with a standard array.
{
Expand Down

0 comments on commit db2e093

Please sign in to comment.