lib: added isNativeError check to assert.js #51250

NiharPhansalkar · 2023-12-21T15:42:22Z

Added the function for compliance with
frameworks such as Jest

NiharPhansalkar · 2023-12-21T15:43:03Z

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.

apapirovski · 2023-12-22T02:53:41Z

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:

node/test/parallel/test-util.js

Line 159 in aa7de74

util.types.isNativeError(new (context('TypeError'))()),

NiharPhansalkar · 2023-12-22T06:34:09Z

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?

apapirovski · 2023-12-22T14:32:07Z

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

NiharPhansalkar · 2023-12-22T15:06:35Z

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)

apapirovski · 2023-12-22T21:05:13Z

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

NiharPhansalkar · 2023-12-24T11:47:02Z

@apapirovski Thank you so much for everything

BridgeAR

These tests are all passing in already released Node.js versions and therefore indicate that it's not testing the right thing.

What we need is something like this:

const context = vm.createContext();
const error = vm.runInContext('new TypeError("foo")', context);

assert.throws(() => assert(false, error), {
  message: 'foo',
  name: 'TypeError'
});

Please always try to focus on what case is not yet covered and how to recreate that situation.

test/parallel/test-assert.js

lib/assert.js

NiharPhansalkar · 2023-12-28T04:04:41Z

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

Added the function for compliance with frameworks such as Jest Fixes: nodejs#50780

Added tests to check if any previous behaviour where instanceof was not throwing a custom error is working with the isNativeError change in assert.ok()

Changed the tests to check for correct throwing of errors and combined if/else into one in assert.js to make code concise.

BridgeAR

LGTM

I left some suggestions to improve the code. We already have a utility function for testing this and we already use it in the file and one test case is redundant.

BridgeAR · 2024-02-12T16:24:03Z

lib/assert.js

@@ -393,7 +394,7 @@ function innerOk(fn, argLen, value, message) {
    } else if (message == null) {
      generatedMessage = true;
      message = getErrMessage(message, fn);
-    } else if (message instanceof Error) {
+    } else if (isNativeError(message) || message instanceof Error) {


Suggested change

} else if (isNativeError(message) || message instanceof Error) {

} else if (isError(message)) {

BridgeAR · 2024-02-12T16:24:22Z

lib/assert.js

@@ -74,6 +74,7 @@ const {
  validateFunction,
 } = require('internal/validators');
 const { fileURLToPath } = require('internal/url');
+const { isNativeError } = internalBinding('types');


Suggested change

const { isNativeError } = internalBinding('types');

BridgeAR · 2024-02-12T16:26:08Z

test/parallel/test-assert.js

@@ -38,7 +38,7 @@ const start = 'Expected values to be strictly deep-equal:';
 const actExp = '+ actual - expected';

 assert.ok(a.AssertionError.prototype instanceof Error,
-          'a.AssertionError instanceof Error');
+  'a.AssertionError instanceof Error');


Suggested change

'a.AssertionError instanceof Error');

'a.AssertionError instanceof Error');

BridgeAR · 2024-02-12T16:26:42Z

test/parallel/test-assert.js

+// Thrown error should be the passed through error instance of the native error
+{
+  const context = vm.createContext();
+  const error = vm.runInContext('new TypeError("custom error")', context);
+
+  assert.throws(() => assert(false, error), {
+    message: 'custom error',
+    name: 'TypeError'
+  });
+}
+


Suggested change

// Thrown error should be the passed through error instance of the native error

{

const context = vm.createContext();

const error = vm.runInContext('new TypeError("custom error")', context);

assert.throws(() => assert(false, error), {

message: 'custom error',

name: 'TypeError'

});

}

BridgeAR · 2024-02-12T16:28:06Z

test/parallel/test-assert.js

+  });
+}
+
+// Thrown error should be the passed through error instance of the native error


Suggested change

// Thrown error should be the passed through error instance of the native error

// Errors created in different contexts are handled as any other custom error

nodejs-github-bot · 2024-05-04T11:16:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58917/

nodejs-github-bot · 2024-05-07T09:51:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59009/

aduh95 · 2024-05-11T14:52:18Z

Linter is failing with Expected indentation of 10 spaces but found 2.

Ceres6 · 2024-11-06T15:53:43Z

Closed by #54776

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. needs-ci PRs that need a full CI run. labels Dec 21, 2023

jasnell approved these changes Dec 22, 2023

View reviewed changes

NiharPhansalkar force-pushed the assert-ok-fix branch 2 times, most recently from ae91ee2 to 2d3421c Compare December 24, 2023 11:46

BridgeAR requested changes Dec 25, 2023

View reviewed changes

test/parallel/test-assert.js Outdated Show resolved Hide resolved

test/parallel/test-assert.js Outdated Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

NiharPhansalkar added 3 commits December 28, 2023 04:19

lib: added isNativeError check to assert.js

a84499f

Added the function for compliance with frameworks such as Jest Fixes: nodejs#50780

test: added test for assert.ok() changes

cf550eb

Added tests to check if any previous behaviour where instanceof was not throwing a custom error is working with the isNativeError change in assert.ok()

test, lib: revised tests and shortened assert code

1de2680

Changed the tests to check for correct throwing of errors and combined if/else into one in assert.js to make code concise.

NiharPhansalkar force-pushed the assert-ok-fix branch from 2d3421c to 1de2680 Compare December 28, 2023 05:43

BridgeAR approved these changes Feb 12, 2024

View reviewed changes

BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 4, 2024

avivkeller mentioned this pull request Jul 21, 2024

assert: use isError instead of instanceof in innerOk to define message type #53980

Merged

BridgeAR closed this Nov 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: added isNativeError check to assert.js #51250

lib: added isNativeError check to assert.js #51250

NiharPhansalkar commented Dec 21, 2023

NiharPhansalkar commented Dec 21, 2023

apapirovski commented Dec 22, 2023

NiharPhansalkar commented Dec 22, 2023 •

edited

Loading

apapirovski commented Dec 22, 2023 •

edited

Loading

NiharPhansalkar commented Dec 22, 2023

apapirovski commented Dec 22, 2023 •

edited

Loading

NiharPhansalkar commented Dec 24, 2023

BridgeAR left a comment

NiharPhansalkar commented Dec 28, 2023

BridgeAR left a comment

BridgeAR Feb 12, 2024

BridgeAR Feb 12, 2024

BridgeAR Feb 12, 2024

BridgeAR Feb 12, 2024

BridgeAR Feb 12, 2024

nodejs-github-bot commented May 4, 2024

nodejs-github-bot commented May 7, 2024

aduh95 commented May 11, 2024

Ceres6 commented Nov 6, 2024

	} else if (isNativeError(message) \|\| message instanceof Error) {
	} else if (isError(message)) {

	'a.AssertionError instanceof Error');
	'a.AssertionError instanceof Error');

	// Thrown error should be the passed through error instance of the native error
	// Errors created in different contexts are handled as any other custom error

lib: added isNativeError check to assert.js #51250

lib: added isNativeError check to assert.js #51250

Conversation

NiharPhansalkar commented Dec 21, 2023

NiharPhansalkar commented Dec 21, 2023

apapirovski commented Dec 22, 2023

NiharPhansalkar commented Dec 22, 2023 • edited Loading

apapirovski commented Dec 22, 2023 • edited Loading

NiharPhansalkar commented Dec 22, 2023

apapirovski commented Dec 22, 2023 • edited Loading

NiharPhansalkar commented Dec 24, 2023

BridgeAR left a comment

Choose a reason for hiding this comment

NiharPhansalkar commented Dec 28, 2023

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Feb 12, 2024

Choose a reason for hiding this comment

BridgeAR Feb 12, 2024

Choose a reason for hiding this comment

BridgeAR Feb 12, 2024

Choose a reason for hiding this comment

BridgeAR Feb 12, 2024

Choose a reason for hiding this comment

BridgeAR Feb 12, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented May 4, 2024

nodejs-github-bot commented May 7, 2024

aduh95 commented May 11, 2024

Ceres6 commented Nov 6, 2024

NiharPhansalkar commented Dec 22, 2023 •

edited

Loading

apapirovski commented Dec 22, 2023 •

edited

Loading

apapirovski commented Dec 22, 2023 •

edited

Loading