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 deep equal check for more Error type #51805

Merged
merged 1 commit into from
May 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions doc/api/assert.md
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,9 @@ An alias of [`assert.ok()`][].
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/51805
description: Error cause and errors properties are now compared as well.
- version: v18.0.0
pr-url: https://github.com/nodejs/node/pull/41020
description: Regular expressions lastIndex property is now compared as well.
Expand Down Expand Up @@ -621,8 +624,8 @@ are also recursively evaluated by the following rules.
both sides are `NaN`.
* [Type tags][Object.prototype.toString()] of objects should be the same.
* Only [enumerable "own" properties][] are considered.
* [`Error`][] names and messages are always compared, even if these are not
enumerable properties.
* [`Error`][] names, messages, causes, and errors are always compared,
even if these are not enumerable properties.
* [Object wrappers][] are compared both as objects and unwrapped values.
* `Object` properties are compared unordered.
* [`Map`][] keys and [`Set`][] items are compared unordered.
Expand Down Expand Up @@ -736,6 +739,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/51805
description: Error cause and errors properties are now compared as well.
- version: v18.0.0
pr-url: https://github.com/nodejs/node/pull/41020
description: Regular expressions lastIndex property is now compared as well.
Expand Down Expand Up @@ -783,8 +789,9 @@ are recursively evaluated also by the following rules.
* [`[[Prototype]]`][prototype-spec] of objects are compared using
the [`===` operator][].
* Only [enumerable "own" properties][] are considered.
* [`Error`][] names and messages are always compared, even if these are not
enumerable properties.
* [`Error`][] names, messages, causes, and errors are always compared,
even if these are not enumerable properties.
`errors` is also compared.
* Enumerable own [`Symbol`][] properties are compared as well.
* [Object wrappers][] are compared both as objects and unwrapped values.
* `Object` properties are compared unordered.
Expand Down
20 changes: 17 additions & 3 deletions lib/internal/util/comparisons.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,23 @@ function innerDeepEqual(val1, val2, strict, memos) {
} else if (isNativeError(val1) || val1 instanceof Error) {
// Do not compare the stack as it might differ even though the error itself
// is otherwise identical.
if ((!isNativeError(val2) && !(val2 instanceof Error)) ||
val1.message !== val2.message ||
val1.name !== val2.name) {
if (!isNativeError(val2) && !(val2 instanceof Error)) {
return false;
}

const message1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'message');
const name1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'name');
const cause1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'cause');
const errors1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'errors');

if ((message1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'message') ||
(!message1Enumerable && val1.message !== val2.message)) ||
(name1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'name') ||
(!name1Enumerable && val1.name !== val2.name)) ||
Comment on lines +247 to +250
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this means now we only compare the name and message fields when they are enumerable, which specificaly opposite from the documentation, and is a breaking change...

Copy link
Contributor Author

@kylo5aby kylo5aby Mar 5, 2024

Choose a reason for hiding this comment

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

As far as I can tell, this means now we only compare the name and message fields when they are enumerable, which specificaly opposite from the documentation, and is a breaking change...

FWIK, As mentioned in ECMAScript 2025, name, message, cause and errors are all non-enumerable, so in the judgement above, I just compared the attributes whether they have same enumerable property, if it's same and they are non-enumerable, then compare the attributes contents.

So if the attributes are enumerable(for example in a custom Error), the next check keyCheck will check them, in the doc change, I have just added errors, and causes attributes, and didn't change name and message, I think its doesn't break the doc, have I missed something?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it was a breaking change, I would expect we would see test failures.

(cause1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'cause') ||
(!cause1Enumerable && !innerDeepEqual(val1.cause, val2.cause, strict, memos))) ||
(errors1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'errors') ||
(!errors1Enumerable && !innerDeepEqual(val1.errors, val2.errors, strict, memos)))) {
return false;
}
} else if (isBoxedPrimitive(val1)) {
Expand Down
23 changes: 23 additions & 0 deletions test/parallel/test-assert-deep.js
Original file line number Diff line number Diff line change
Expand Up @@ -1111,6 +1111,29 @@ assert.throws(
assert.notDeepStrictEqual(err, err2);
}

// Check for Errors with cause property
{
const e1 = new Error('err', { cause: new Error('cause e1') });
const e2 = new Error('err', { cause: new Error('cause e2') });
assertNotDeepOrStrict(e1, e2, AssertionError);
assertNotDeepOrStrict(e1, new Error('err'), AssertionError);
assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }));
}

// Check for AggregateError
{
const e1 = new Error('e1');
const e1duplicate = new Error('e1');
const e2 = new Error('e2');

const e3 = new AggregateError([e1duplicate, e2], 'Aggregate Error');
const e3duplicate = new AggregateError([e1, e2], 'Aggregate Error');
const e4 = new AggregateError([e1], 'Aggregate Error');
assertNotDeepOrStrict(e1, e3, AssertionError);
assertNotDeepOrStrict(e3, e4, AssertionError);
assertDeepAndStrictEqual(e3, e3duplicate);
}

// Verify that `valueOf` is not called for boxed primitives.
{
const a = new Number(5);
Expand Down
Loading