FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

assert: add deep equal check for more Error type - #51805

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:assert-error-type-compare
May 12, 2024
Merged

assert: add deep equal check for more Error type#51805
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
kylo5aby:assert-error-type-compare

Conversation

Copy link
Copy Markdown
Contributor

add deep equal check for the cases below:

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

Fixes: #51793

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 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/internal/util/comparisons.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/internal/util/comparisons.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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;
}

Comment thread test/parallel/test-assert-deep.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 force-pushed the assert-error-type-compare branch 2 times, most recently from bd5f30d to bcf4e54 Compare February 20, 2024 04:13

Copy link
Copy Markdown
Contributor Author

@BridgeAR Thanks for your suggestion!

Copy link
Copy Markdown
Contributor Author

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

Comment thread lib/internal/util/comparisons.js Outdated
Comment on lines 247 to 250

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

kylo5aby Mar 5, 2024
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor Author

@legendecas PTAL

legendecas left a comment
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread doc/api/assert.md Outdated
Comment thread test/parallel/test-assert-deep.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread test/parallel/test-assert-deep.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
assertDeepAndStrictEqual(e3, e3duplicate, AssertionError);
assertDeepAndStrictEqual(e3, e3duplicate);

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
Comment thread doc/api/assert.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Resolved. Thanks for the reminder

kylo5aby force-pushed the assert-error-type-compare branch from 5f48ab0 to 7562ac6 Compare March 19, 2024 01:55
aduh95 added author ready PRs that have at least one approval, no outstanding review comments, 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

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

Landed in c8a4f70

targos pushed a commit that referenced this pull request May 13, 2024
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
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
PR-URL: nodejs#51805
Fixes: nodejs#51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert Issues and PRs related to the assert subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

deepStrictEqual and notDeepStrictEqual do not compare Error causes nor AggregateError errors arrays

6 participants


Back | FazBrowse Home | New Git URL