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

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

Closed
Zamralik opened this issue Feb 17, 2024 · 6 comments · Fixed by #51805
Labels
assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.

Comments

@Zamralik
Copy link

Version

v20.9.0

Platform

Linux DESKTOP-8H17SD6 5.15.133.1-microsoft-standard-WSL2 #1 SMP Thu Oct 5 21:02:42 UTC 2023 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

  • Compare 2 Error one with a cause and one without
  • Compare 2 Error one with different causes
  • Compare 2 AggregateError with different array of errors

These 4 calls should not throw.
Inversely, replacing notDeepStrictEqual by deepStrictEqual should throw for all.

import { notDeepStrictEqual } from "node:assert";

notDeepStrictEqual(
    new Error('Test'),
    new Error('Test', { cause: new Error('Expected Cause') }),
);

notDeepStrictEqual(
    new Error('Test', { cause: new Error('Unexpected Cause') }),
    new Error('Test'),
);

notDeepStrictEqual(
    new Error('Test', { cause: new Error('Unexpected Cause') }),
    new Error('Test', { cause: new Error('Expected Cause') }),
);

notDeepStrictEqual(
    new AggregateError([], 'Aggregate Test'),
    new AggregateError([new Error('Expected Child')], 'Aggregate Test'),
);

notDeepStrictEqual(
    new AggregateError([new Error('Unexpected Child')], 'Aggregate Test'),
    new AggregateError([], 'Aggregate Test'),
);

notDeepStrictEqual(
    new AggregateError([new Error('Unexpected Child')], 'Aggregate Test'),
    new AggregateError([new Error('Expected Child')], 'Aggregate Test'),
);

How often does it reproduce? Is there a required condition?

Every time

What is the expected behavior? Why is that the expected behavior?

notDeepStrictEqual should throw when the Error's cause / AggregateError's errors match perfectly recursively.
deepStrictEqual should throw when the Error's cause / AggregateError's errors do not match perfectly recursively.

What do you see instead?

notDeepStrictEqual throw when the Error's cause / AggregateError's errors match perfectly recursively.
deepStrictEqual do not throw when the Error's cause / AggregateError's errors do not match perfectly recursively.

Additional information

No response

@kylo5aby
Copy link
Contributor

As mentioned in deepstrictequalactual, for Error type, it compares names, messages and enumerable "own" properties.

cause, message, and name, are both non-enumerable properties. However, as mentioned in ECMA2024, cause triggers some special operations during the creation of an Error instance, so I believe cause serves as another distinguishing attribute among different Error, much like message and name. I'm unsure if others agree with this perspective, and I'm willing to submit a PR if feasible.

@Zamralik
Copy link
Author

Zamralik commented Feb 18, 2024

I use AggregateError and Error's cause to create a tree to provide an explicit trail of what was wrong and where.

Being able to recursively compare the Error's cause/Aggregate's errors in the tests is more thorough that no info on the reason is missing.

@BridgeAR
Copy link
Member

I agree with @zhenweijin that we should just add a special handling for it. It should be pretty straight forward to add that check.

@BridgeAR BridgeAR added good first issue Issues that are suitable for first-time contributors. assert Issues and PRs related to the assert subsystem. feature request Issues that request new features to be added to Node.js. labels Feb 19, 2024
@BridgeAR BridgeAR added the util Issues and PRs related to the built-in util module. label Feb 19, 2024
@abidjappie
Copy link

abidjappie commented Feb 19, 2024

I'm interested in looking at this issue if it become available 🙇 (I see it's still in Triage)

@Elham-EN
Copy link

Hi, can you assign me to this issue?

@BridgeAR BridgeAR removed the good first issue Issues that are suitable for first-time contributors. label Feb 21, 2024
@BridgeAR
Copy link
Member

There's an open PR, so it's almost done.

nodejs-github-bot pushed a commit that referenced this issue May 12, 2024
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this issue May 13, 2024
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jun 17, 2024
PR-URL: #51805
Fixes: #51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sophoniie pushed a commit to sophoniie/node that referenced this issue Jun 20, 2024
PR-URL: nodejs#51805
Fixes: nodejs#51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
PR-URL: nodejs#51805
Fixes: nodejs#51793
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@avivkeller avivkeller moved this from Awaiting Triage to Done in Node.js feature requests Jun 28, 2024
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. feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants