nodejs
`assert.ok()` throwing `AssertionError` instead of provided `Error` object
Version
v20.9.0
Platform
Linux executive 6.5.0-10-generic #10-Ubuntu SMP PREEMPT_DYNAMIC Fri Oct 13 13:49:38 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
assert
What steps will reproduce the bug?
assert.ok() docs says:
If the message parameter is an instance of an Error then it will be thrown instead of the AssertionError.
I've called to it with an Error instance as second argument, and instead of being thrown that error, it's being thrown an AssertionError with it's message field set to the message field of the provided error.
Not sure what's the correct solution here, based on docs and common sense, the provided error should be thrown, but based on homogeneity, an AssertionError should be thrown (current behaviour) so it always throw an AssertionError no matter what it's provided as second argument...
How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior? Why is that the expected behavior?
Not sure if it's a bug, or a documentation issue.
What do you see instead?
AssertionError is thrown, with provided Error message.
Additional information
No response
https://github.com/nodejs/node/blob/25fdb48876e1c1f2c7e16506b2e1c5bf4b404ecf/lib/assert.js#L396-L398
It clearly re-thrown if the message is instanceof Error, but instanceof checking may not be true depends on testing framework.
A minimal repo shown it is working correctly.
import { ok } from 'assert/strict'
ok(false, Error('here is error'))
This working as expected and will throw to avoid reading a Node.js module, as doing so could potentially result in misleading errors being thrown from module.
instanceofchecking may not be true depends on testing framework
My fault... I'm using Jest, and forget it has problems with instanceof and builtin classes 🤦🏻. Could make it sense to use ducktyping and check for Error objects fields? Based on spec, message, name and stack can made us pretty sure they are Error objects themselves...
We could use the same algorithm as util.isError:
https://github.com/nodejs/node/blob/1858341377c268c5ffce9345517dc07b0e6c240a/lib/util.js#L167-L169
We could use the same algorithm as
util.isError
It's deprecated, and also it would not work for Error child classes.
The function is deprecated, but not what it does. It works fine with child classes.
It works fine with child classes.
I think it wouldn't, because on child classes, the prototype constructor name would be different of Error string, so the comparison would fail and we would get to the e instanceof Error, leading to the same Jest error we have here in this issue.
We can also add a check that looks util.types.isNativeError(). That is how assert itself checks for errors next to instanceof.
Seeing the discussion, I will try to implement both the methods mentioned to see if it actually throws the required error. Will update accordingly?
The wrong Error class is being dispatched due to using Jest, it's a bug there due to how it manage native classes. Proper solution should be done there, but in addition to that, a isNativeError() function can be useful too, at least to bypass the problem here and in other places.
So you initially want to check if a function similar to isNativeError() returns true, and if it does, just directly throw that error instead of bothering with how instanceof is treated?
@piranna Is there any way to get Jest to use my local nodejs? i.e the node I have in node/out/Release/node?
I tried searching around but can't find much. Maybe you have tried this before?
I have compiled my node, how do I launch Jest with it? Is there some particular config option for it? Or a command line flag?
I noticed that this issue is stalled even though there is an almost accepted PR that solves the problem. I addressed the comments and fixed the problem that was blocking the CI (we'll find out on the next CI run, hehe 🐍 ).
PR: #53980
P.S.: I added the author of the PR and the commenter as co-authors in the commit.
