node
node copied to clipboard
nodejs
lib: added isNativeError check to assert.js
Added the function for compliance with frameworks such as Jest
Fixes: #50780
I want to ask what kinds of tests can be written for this fix? The actual problem was occuring for frameworks such as Jest. Native code works just fine.
I want to ask what kinds of tests can be written for this fix? The actual problem was occuring for frameworks such as Jest. Native code works just fine.
Look for how other tests that utilize isNativeError are testing this behavior, which will give you a hint on how to create an Error that doesn't pass an instanceof check but passes isNativeError check.
Here's one helpful link: https://github.com/nodejs/node/blob/aa7de747b7743fa530b508365f28533bf29078d9/test/parallel/test-util.js#L159
The test for this is already being covered in the following:
https://github.com/nodejs/node/blob/ba957a61f83365d9df7126c23a1c78c320061414/test/parallel/test-assert.js#L50C5-L50C46
Where if the code is now run with Jest, instead of an AssertionError, the provided error will be thrown.
Do I still need to add any tests?
Where if the code is now run with Jest, instead of an
AssertionError, the provided error will be thrown.Do I still need to add any tests?
You need a test in Node's test suite that would fail w/ the old version of the code and succeed w/ the new one. Right now that doesn't exist. Running it with Jest isn't relevant since we don't run our test suite w/ Jest.
Writing tests is quite straightforward: https://github.com/nodejs/node/blob/main/doc/contributing/writing-tests.md
I am a little confused @apapirovski. If you go through the discussion of the issue, you can see that the problem was never with NodeJS directly. The code was working fine when running with node even without the change I have made. The problem was occuring with frameworks such as Jest who have some issue with the instanceof operator.
So, this change is just to help out with that, if just in case someone does start using Jest (or some framework like it) and uses assert.ok() to throw a custom error, they should not fall into trouble.
So, I don't think it is possible for me to write a test which will fail for a version before this PR but pass for a version after this PR since there never was an issue in Node directly. (So it will pass even if this PR is not put into action)
Tests for Node.js are meant to reproduce the issue that we saw in another library. Here's a line of code that would fail on the Node.js before this change and one that will pass now:
assert.ok(false, new (context('Error'))('Custom Error'))
Pre-and-post output looks like this:
Uncaught AssertionError [ERR_ASSERTION]: Error: Custom Error
at REPL3:1:8
at Script.runInThisContext (node:vm:129:12)
at REPLServer.defaultEval (node:repl:566:29)
at bound (node:domain:421:15)
at REPLServer.runBound [as eval] (node:domain:432:12)
at REPLServer.onLine (node:repl:893:10)
at REPLServer.emit (node:events:538:35)
at REPLServer.emit (node:domain:475:12)
at REPLServer.Interface._onLine (node:readline:487:10)
at REPLServer.Interface._line (node:readline:864:8) {
generatedMessage: false,
code: 'ERR_ASSERTION',
actual: false,
expected: true,
operator: '=='
}
Post:
Uncaught Error: Custom Error
@BridgeAR I will make the changes, however can you please tell me if I am able to understand the reason for this?
My current commit, it checks for e to be an instanceof Error, however, this does not tell us whether what was thrown was an assertion error or a specific user mentioned error, which is why my tests are incorrect.
Do I understand?
