assert: handle errors properly with deep*Equal #15001
Conversation
nodejs-github-bot
added
the
assert
|
@TimothyGu the stack is explicitly not checked as it would always differ depending on where you created the Error. The object itself can be completely identical otherwise and this is also how it is done in most libraries (I am not aware that any checks for the stack). |
BridgeAR
added
the
semver-major
doc/api/assert.md
Outdated
| not enumerable: | ||
|
|
||
| ```js | ||
| // WARNING: This does not throw an AssertionError! | ||
| assert.deepEqual(Error('a'), Error('b')); | ||
| assert.deepEqual(/a/, {}); |
There was a problem hiding this comment.
I wonder if we can either come up with a better example or if it's OK to just remove the example entirely. The current one makes it so clear that intuition would be wrong, but this one not so much. This seems more akin to assert.deepEqual('', 0) than the current example.
There was a problem hiding this comment.
assert.deepEqual(new Date(), new Uint8Array())?
There was a problem hiding this comment.
Oh , yeah, I think it's definitely less obvious with a Date object that there aren't enumerable properties!
I think something like this might be even better:
assert.deepEqual(new Date(), /a/);My thinking is that those are two objects where the absence of enumerable properties is easy to forget. So that result is genuinely surprising at a glance.
In contrast, the idea that an empty object or an empty array don't have enumerable properties is more obvious.
I might be overthinking this a bit. :-D
|
I updated the example and also made the check in general more strict as it now also checks for weird inheritance. That was a oversight before and now it will handle those properly as well. While thinking about it I would actually do not consider this a semver-major but a bug fix because most people would anticipate that errors would be compared. Other opinions about this? |
BridgeAR
removed
the
semver-major
|
I actually removed the weird inheritance again because I think we should not support that. |
|
Is this semver-major? |
|
@jasnell I count this as a bugfix and it would only now show errors in cases where people actually wanted to get an error as you would probably otherwise not use deepEqual on errors in the first place otherwise. |
|
Ping @jasnell |
|
I can go with that :) |
BridgeAR
force-pushed
the
assert-errors
branch
from
6b7d694
to
34364ea
Compare
|
Rebased due to conflicts |
|
It would be nice to get some more reviews @nodejs/collaborators |
| @@ -230,6 +237,10 @@ function looseDeepEqual(actual, expected) { | |||
| if (isRegExp(actual) && isRegExp(expected)) { | |||
| return areSimilarRegExps(actual, expected); | |||
| } | |||
| if (actual instanceof Error && expected instanceof Error) { | |||
| if (actual.message !== expected.message || actual.name !== expected.name) | |||
There was a problem hiding this comment.
Could you compare code as well?
There was a problem hiding this comment.
code will be compared if it is set by the user as a non enumerable property. This is only intended for the non enumerable properties message and name from the default Error classes and not for userland sub-classing.
There was a problem hiding this comment.
Ahh Ok. The keys of expected are the assertion demand.
(I was thinking of the case of trying to assert a NodeError where .code comes from the prototype)
So will this be an effective check?
assert.deepStrictEqual(someNodeError, Object.assign(new Error(msg), {code: ERR_CODE}));There was a problem hiding this comment.
This would fail because the left prototype is not the same as the right one.
|
Landed in 204d94f |
PR-URL: #15001 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#15001 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #15001 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #15001 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Should this be backported to |
|
#13862 should be backported first. I opened a PR for that and I will look into backporting this after it landed. |
|
Hm, there are quite a few commits that should be backported before this one. @MylesBorins if you want that I could make a single backport with all assert commits that I guess make sense to backport. |
Since the assert module is not locked anymore it became much better over time (at least the strict part). But it still does not compare Errors and that is really a pain. I feels natural that this would throw.
This introduces such a check. Fixes #3122
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
assert