assert.ok() throwing AssertionError instead of provided Error object

[piranna]

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

[climba03003]

node/lib/assert.js

Lines 396 to 398 in 25fdb48

	} else if (message instanceof Error) {
	throw message;
	}

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

[MrJithil]

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.

[piranna]

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

[targos]

We could use the same algorithm as util.isError:

node/lib/util.js

Lines 167 to 169 in 1858341

	function isError(e) {
	return ObjectPrototypeToString(e) === '[object Error]' || e instanceof Error;
	}

[piranna]

We could use the same algorithm as util.isError

It's deprecated, and also it would not work for Error child classes.

[targos]

The function is deprecated, but not what it does. It works fine with child classes.

[piranna]

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.

[targos]

If you don't believe me, try it

[BridgeAR]

We can also add a check that looks util.types.isNativeError(). That is how assert itself checks for errors next to instanceof.

[piranna]

I like the idea.

[NiharPhansalkar]

Hello, I would like to try and work on this issue.

[piranna]

What do you need?

[NiharPhansalkar]

Seeing the discussion, I will try to implement both the methods mentioned to see if it actually throws the required error. Will update accordingly?

[piranna]

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.

[NiharPhansalkar]

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]

I think so, yes.

[NiharPhansalkar]

@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?

[piranna]

Compile your Node.js, and launch Jest with It.

[NiharPhansalkar]

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?

[targos]

out/Release/node path/to/jest/cli.js

[ignaciosuarezquilis]

Hi, can I work on this issue?

[piranna]

Hi, can I work on this issue?

Sure, go for it :-)

[Naveen-2021ucp1387]

is this issue still open ?

[piranna]

Yes, it's still a problem.

[pmarchini]

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.

[piranna]

Great, good work @pmarchini :-)