assert: add deep equal check for more Error type by kylo5aby · Pull Request #51805 · nodejs/node

kylo5aby · 2024-02-19T09:08:53Z

add deep equal check for the cases below:

Error with cause property: new Error('msg', { cause: xxx })
AggregateError: new AggregateError([err1, err2, ...], 'msg')

BridgeAR

This is already looking very promising!

The documentation still needs an update (we describe what properties we check in addition to enumerable ones) and I left improvement suggestions for edge cases.

BridgeAR · 2024-02-19T09:45:39Z

lib/internal/util/comparisons.js

+        !innerDeepEqual(val1.cause, val2.cause, strict, memos)) {
      return false;
    }
+    if (val1 instanceof AggregateError && val2 instanceof AggregateError) {


The instanceof check will work in most cases while it would not detect AggregateErrors created in a different context/realm (e.g., by creating errors in a context created by vm.createContext()). I believe it is fine to just always check for this property, in case it is not already an enumerable property.

This check would currently miss the case, if either side is an AggregateError and the other is not. That could be addressed in a similar way as I suggested above about the enumerable property.

BridgeAR · 2024-02-19T10:01:22Z

lib/internal/util/comparisons.js

        val1.message !== val2.message ||
-        val1.name !== val2.name) {
+        val1.name !== val2.name ||
+        !innerDeepEqual(val1.cause, val2.cause, strict, memos)) {


This is definitely the right check as long the property is non-enumerable. If it is enumerable, we'd do the check twice and this could be costly and a lot of errors are created in the wild with a cause property set as an enumerable one.

In addition, we should also verify that the properties have the same enumerability. We don't yet do that for name and message, but that would be beneficial.

const cause1Enumerable = ObjectPrototypePropertyIsEnumerable(val1, 'cause'); if (cause1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'cause') || !cause1Enumerable && !innerDeepEqual(val1.cause, val2.cause, strict, memos)) { return false; }

BridgeAR · 2024-02-19T10:06:30Z

test/parallel/test-assert-deep.js

+  const e3 = new Error('e1');
+  const e4 = new AggregateError([e1, e2], 'Aggregate Error');
+  const e5 = new AggregateError([e1], 'Aggregate Error');
+  const e6 = new AggregateError([e1, e2], 'Aggregate Error');


Nit (just a general suggestion without need to follow-up upon): the variable names could be changed in a way that it's clearer what they stand for. E.g., this one could be:

Suggested change

const e6 = new AggregateError([e1, e2], 'Aggregate Error');

const e4duplicate = new AggregateError([e1, e2], 'Aggregate Error');

That way it would be easy to understand in the comparison that the entries are meant to be identical.

kylo5aby · 2024-02-20T06:05:19Z

@BridgeAR Thanks for your suggestion!

kylo5aby · 2024-02-23T07:21:36Z

@BridgeAR, I have update based on Non-Enumerable properties on Error, PTAL feel free

atlowChemi · 2024-03-04T15:02:21Z

lib/internal/util/comparisons.js

+    if ((message1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'message') ||
+          (!message1Enumerable && val1.message !== val2.message)) ||
+        (name1Enumerable !== ObjectPrototypePropertyIsEnumerable(val2, 'name') ||
+          (!name1Enumerable && val1.name !== val2.name)) ||


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...

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?

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

kylo5aby · 2024-03-18T03:04:41Z

@legendecas PTAL

legendecas

Would you mind adding an item to the change list in the YAML frontmatter as well? Such as:

<!-- YAML
changes:
  - version: REPLACEME
    pr-url: https://github.com/nodejs/node/pull/51805
    description: Error cause property is now compared as well.
--->

doc/api/assert.md

legendecas · 2024-03-18T07:57:41Z

test/parallel/test-assert-deep.js

+  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') }), AssertionError);


assertDeepAndStrictEqual only accepts two arguments.

Suggested change

assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }), AssertionError);

assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }));

legendecas · 2024-03-18T07:57:49Z

test/parallel/test-assert-deep.js

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


Suggested change

assertDeepAndStrictEqual(e3, e3duplicate, AssertionError);

assertDeepAndStrictEqual(e3, e3duplicate);

legendecas · 2024-03-18T16:49:39Z

doc/api/assert.md

-* [`Error`][] names and messages are always compared, even if these are not
-  enumerable properties.
+* [`Error`][] names, messages and causes are always compared, even if these are
+  not enumerable properties. For `AggregateError`, non-enumerable property


I believe the actual behavior unconditionally compares the errors property regardless of whether the error instance is an AggregateError or not. This could be reworded as

* [`Error`][] names, messages, causes, and errors are always compared, even if these are not enumerable properties.

Resolved. Thanks for the reminder

nodejs-github-bot · 2024-05-11T15:33:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59119/

nodejs-github-bot · 2024-05-12T08:58:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59158/

nodejs-github-bot · 2024-05-12T18:03:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59181/

nodejs-github-bot · 2024-05-12T19:21:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59187/

nodejs-github-bot · 2024-05-12T19:46:14Z

Landed in c8a4f70

PR-URL: #51805 Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#51805 Fixes: nodejs#51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Feb 19, 2024

BridgeAR reviewed Feb 19, 2024

View reviewed changes

kylo5aby force-pushed the assert-error-type-compare branch 2 times, most recently from bd5f30d to bcf4e54 Compare February 20, 2024 04:13

atlowChemi reviewed Mar 4, 2024

View reviewed changes

legendecas reviewed Mar 18, 2024

View reviewed changes

doc/api/assert.md Outdated Show resolved Hide resolved

legendecas reviewed Mar 18, 2024

View reviewed changes

legendecas added the assert Issues and PRs related to the assert subsystem. label Mar 18, 2024

kylo5aby force-pushed the assert-error-type-compare branch 3 times, most recently from f93555c to 5f48ab0 Compare March 18, 2024 10:30

legendecas reviewed Mar 18, 2024

View reviewed changes

assert: add deep equal check for more Error type

7562ac6

kylo5aby force-pushed the assert-error-type-compare branch from 5f48ab0 to 7562ac6 Compare March 19, 2024 01:55

legendecas approved these changes Mar 19, 2024

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 11, 2024

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 12, 2024

nodejs-github-bot merged commit c8a4f70 into nodejs:main May 12, 2024

targos pushed a commit that referenced this pull request May 13, 2024

assert: add deep equal check for more Error type

0f5716c

PR-URL: #51805 Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

targos mentioned this pull request May 13, 2024

v22.2.0 release proposal #52971

Merged

This was referenced Jun 12, 2024

The "cause" property of Error is ignored when matching vitest-dev/vitest#5697

Closed

fix(expect)!: check more properties for error equality vitest-dev/vitest#5876

Merged

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024

assert: add deep equal check for more Error type

e19c60c

PR-URL: #51805 Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024

assert: add deep equal check for more Error type

7d0c4b5

PR-URL: #51805 Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024

assert: add deep equal check for more Error type

227093b

PR-URL: #51805 Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024

assert: add deep equal check for more Error type

a064469

PR-URL: nodejs#51805 Fixes: nodejs#51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024

assert: add deep equal check for more Error type

715ddb4

PR-URL: nodejs#51805 Fixes: nodejs#51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

hi-ogawa mentioned this pull request Oct 8, 2024

Odd assertion error message when deep equality failed due to Error.cause #55310

Closed

	const e6 = new AggregateError([e1, e2], 'Aggregate Error');
	const e4duplicate = new AggregateError([e1, e2], 'Aggregate Error');

	assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }), AssertionError);
	assertDeepAndStrictEqual(e1, new Error('err', { cause: new Error('cause e1') }));

	assertDeepAndStrictEqual(e3, e3duplicate, AssertionError);
	assertDeepAndStrictEqual(e3, e3duplicate);

Uh oh!

Conversation

kylo5aby commented Feb 19, 2024

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylo5aby commented Feb 20, 2024

Uh oh!

kylo5aby commented Feb 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylo5aby Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylo5aby commented Mar 18, 2024

Uh oh!

legendecas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 11, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

nodejs-github-bot commented May 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kylo5aby Mar 5, 2024 •

edited

Loading

legendecas left a comment •

edited

Loading