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

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

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.
constcause1Enumerable=ObjectPrototypePropertyIsEnumerable(val1,'cause');if(cause1Enumerable!==ObjectPrototypePropertyIsEnumerable(val2,'cause')||!cause1Enumerable&&!innerDeepEqual(val1.cause,val2.cause,strict,memos)){returnfalse;}

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

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
conste6=newAggregateError([e1,e2],'Aggregate Error');
conste4duplicate=newAggregateError([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

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

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

assertDeepAndStrictEqual only accepts two arguments.
Suggested change
assertDeepAndStrictEqual(e1,newError('err',{cause: newError('cause e1')}),AssertionError);
assertDeepAndStrictEqual(e1,newError('err',{cause: newError('cause e1')}));

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

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

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

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: #51805Fixes: #51793 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

PR-URL: nodejs#51805Fixes: 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 bcf4e54Compare February 20, 2024 04:13

atlowChemi reviewed Mar 4, 2024
View reviewed changes

legendecas reviewed Mar 18, 2024
View reviewed changes

Comment threaddoc/api/assert.md Outdated

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 5f48ab0Compare 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 7562ac6Compare 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-actionsBot 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:mainMay 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: #51805Fixes: #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 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
227093b
PR-URL: #51805Fixes: #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#51805Fixes: 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

	conste6=newAggregateError([e1,e2],'Aggregate Error');
	conste4duplicate=newAggregateError([e1,e2],'Aggregate Error');

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

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

Uh oh!

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!

kylo5abyMar 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

kylo5abyMar 5, 2024•
edited
Loading

legendecas left a comment •
edited
Loading