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