fix `common.expectsError` type checking #13686

refack · 2017-06-14T19:49:55Z

Fixes: #13682

More exact types for Errors in the tests
fix common.expectsError type checking

/cc @cjihrig @nodejs/testing

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

refack · 2017-06-14T19:50:52Z

I'm about 50% there. Looking for general feedback.

jasnell · 2017-06-14T19:53:51Z

test/parallel/test-dgram-bind.js

@@ -31,7 +32,7 @@ socket.on('listening', common.mustCall(() => {
    socket.bind();
  }, common.expectsError({
    code: 'ERR_SOCKET_ALREADY_BOUND',
-    type: Error,
+    type: e.Error,


I'd rather this not check against the internal errors objects. These need to be recognizable as normal Error, TypeError and RangeError instances.

jasnell · 2017-06-14T19:55:58Z

test/parallel/test-fs-watchfile.js


+const expectedErr = common.expectsError({
+  type: Error,


Please only use common.expectsError() with errors that have been migrated to internal/errors. The reason is so that the existing places in the tests where the error message is checked can be more easily found when doing the migration. If the tests already use common.expectsError(), then it's far more likely that whoever is doing the migrating will miss those.

jasnell · 2017-06-14T19:56:15Z

test/parallel/test-fs-watchfile.js

+  Date.UTC(1970, 0, 1, 0, 0, 0),            // atime
+  Date.UTC(1970, 0, 1, 0, 0, 0),            // mtime
+  Date.UTC(1970, 0, 1, 0, 0, 0),            // ctime
+  Date.UTC(1970, 0, 1, 0, 0, 0)             // birthtime


Unrelated whitespace changes

jasnell · 2017-06-14T19:57:11Z

test/parallel/test-internal-errors.js

+assert.strictEqual(
+  errors.message('ERR_INVALID_URL_SCHEME', ['file']),
+  'The URL must be of scheme file'
+);


why change the formatting on these?

jasnell · 2017-06-14T19:59:01Z

Generally not a fan of updating the common.expectsError() checks to test against type: e.Error and such. The fact that those are internal errors is largely inconsequential. The check needs to be verifying that they are recognizable as the externally facing error types (Error, TypeError, and RangeError).

I'm definitely -1 on this PR as it currently stands.

refack · 2017-06-14T20:29:56Z

Generally not a fan of updating the common.expectsError() checks to test against type: e.Error and such. The fact that those are internal errors is largely inconsequential. The check needs to be verifying that they are recognizable as the externally facing error types (Error, TypeError, and RangeError).

I'm definitely -1 on this PR as it currently stands.

Ok. Thanks for the feedback. The other solution I had in mind is to use error.constructor.name and bypass the inheritance chain.

cjihrig · 2017-06-14T20:49:35Z

The fact that those are internal errors is largely inconsequential.

I don't know if I'd say inconsequential. We've given up some things by using custom errors.

jasnell · 2017-06-14T21:34:41Z

We've given up some things by using custom errors

Like what?

refack · 2017-06-14T23:18:03Z

Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6638/
@jasnell PTAL

using constructor.name I found some bugs in /parallel/test-internal-errors.js
Based on fix common.expectsError type checking #13686 (comment) I added a check that expectsError is used with internal errors (a bit more lax to allow AssersionError and RangeError)
found two places that used it for "regular" errors: parallel/test-fs-realpath.js & parallel/test-http-request-invalid-method-error.js

refack · 2017-06-14T23:18:43Z

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

cjihrig · 2017-06-15T00:01:47Z

We've given up some things by using custom errors

Like what?

Correctly identifying an error seems to be very hard to do. Even our internal isError() function partially depends on toString() behavior, which can be faked. By moving to custom errors it appears that we can't rely on the constructor (unless we identify the error as a custom internal error) or V8's IsNativeError().

jasnell · 2017-06-15T00:09:50Z

The following works well

const errors = require('internal/errors');
const util = require('util');
const err = new err.Error('ERR_ASSERTION');

util.isError(err);     // true
err instanceof Error;  // true

err.constructor.prototype instanceof Error; // true

cjihrig · 2017-06-15T23:20:59Z

Replace:

const err = new errors.Error('ERR_ASSERTION');

With:

const err = new errors.TypeError('ERR_ASSERTION');

And all three of those still report true. That's currently broken in common.expectsError(), assuming that function should be able to distinguish Error from TypeError. Based on recent efforts in our test suite, I think thats an important feature.

refack · 2017-06-16T02:28:18Z

Replace:

const err = new errors.Error('ERR_ASSERTION');
With:

const err = new errors.TypeError('ERR_ASSERTION');
And all three of those still report true. That's currently broken in common.expectsError(), assuming that function should be able to distinguish Error from TypeError. Based on recent efforts in our test suite, I think thats an important feature.

Well you two are commenting in a PR that fixes that 😉

jasnell · 2017-06-16T14:46:51Z

And all three of those still report true.

As one should expect them to. This is not specific to common.expectsError(). Consider the following:

assert.throws(() => { throw new TypeError() }, Error);

That passes because assert.throws() uses instanceof to verify that the thrown error is Error, and since TypeError is a subclass of Error, everything is good.

The following is all correct behavior:

const err = new err.TypeError('ERR_ASSERTION');

util.isError(err);     // true
err instanceof Error;  // true

err.constructor.prototype instanceof Error; // true

jasnell · 2017-06-16T14:48:04Z

test/parallel/test-fs-realpath.js

@@ -225,7 +225,7 @@ function test_cyclic_link_protection(callback) {
  });
  assert.throws(() => {
    fs.realpathSync(entry);
-  }, common.expectsError({ code: 'ELOOP', type: Error }));
+  }, /^Error: ELOOP: too many symbolic links encountered, stat/);


I'm -1 on changing this one. The check here is correct, tho the message check can be added.

You asked that "regular" errors checks won't use common.expectsError.
I'll check again.

My apologies, I mean for errors that do not currently have a code property. These do.

jasnell · 2017-06-16T14:49:53Z

test/common/index.js

-    if (type !== undefined)
-      assert(error instanceof type,
-             `${error} is not the expected type ${type}`);
+    if (expectedTypeName) {


I'm not seeing why this increase in complexity is necessary. The instanceof check works just fine.

Because

var e = new TypeError(); e instanceof Error === true;

but that's ok. I don't see that it needs to be stricter than that

For the purposes of our own test suite, I'd expect a TypeError assertion to only work with TypeErrors.

I'm with @cjihrig.

assert(err instanceof Error)

Does not protect for a change in the type of err. The code could change it from Error to TypeError and the test would pass. Even changes from TypeError to RangeError would pass.
Those are semver-major changes that we don't assert.

coreAPI.function(arg, (err, ret) => { if (err instanceof TypeError) console.log('bad programer!'); if (err instanceof Error) console.log('oops'); }

if I do common.expectsError({type: TypeError}) it will only be true if the error is a TypeError (or a subclass of TypeError. If you'd like a stricter check, perhaps add a new argument like {strictType: TypeError} so that tests that might happen to require absolutely strict checking can do so. Or, perhaps even allow type: to take a function so stricter checking can be optionally performed.

or a subclass of TypeError

That is the problem. In a recent PR, someone used common.expectsError() with Error, when they meant TypeError. The test should have failed on this.

In our test suite we almost exclusively want the strictest error checks possible. Even if we were to add strictType (which I don't think we should), how can we achieve that without exposing the fact that we're using the internal errors?

Yes, the downcast case is covered, but the upcast is not (api that throw Error changed to throw TypeError, test still pass). I found one such case.
You know what will happen, we'll add strictType then we'll deprecate type and add a lint rule...
If you look at the change-set of the PR now, you'll see that enforcing strict type needs no test changes, and will protect from Errors being accidently specialized.

refack · 2017-06-19T17:16:18Z

Just to sum up, there are two levels of new assertions in 4 commits:

ceef6c7 - expectsError({type}) checks type name, so expectError({type: Error})(new TypeError()) will fail
2012d3f - fix 2 bugs in test-internal-errors.js
b0c8375 - and add more assertions to that file
168fc477d1f9066f47697e7c9e02bd34e763e25e - (based on fix common.expectsError type checking #13686 (comment)) - Check that expectsError is used for node internal errors (for easy deduction of non-converted Error code paths from test code)
~~f8405bc - Fix 2 case that violate (3)~~

/cc @nodejs/testing

jasnell · 2017-06-19T17:35:52Z

test/parallel/test-internal-errors.js

@@ -215,10 +241,11 @@ assert.strictEqual(errors.message('ERR_MISSING_ARGS', ['name']),
 assert.strictEqual(errors.message('ERR_MISSING_ARGS', ['name', 'value']),
                   'The "name" and "value" arguments must be specified');
 assert.strictEqual(errors.message('ERR_MISSING_ARGS', ['a', 'b', 'c']),
-                   'The "a", "b", and "c" arguments must be specified');
+  'The "a", "b", and "c" arguments must be specified');


Unnecessary whitespace change?

jasnell · 2017-06-19T17:37:34Z

I'm generally still -1 on the stricter error type checking in common.expectsError() as I do not think that it is necessary.

refack · 2017-06-19T17:46:14Z

Sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/6738/nodes=rhel72-s390x/

BridgeAR · 2018-01-06T03:37:28Z

Landed in 15016f2, 29d99c1, f26cabb

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins · 2018-01-09T09:10:07Z

This will need to be manually backported to v9.x as it is breaking a thing or two

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Backport-PR-URL: #19579 PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

The function should strictly test for the error class and only accept the correct one. Any other error class should fail. Backport-PR-URL: #19579 PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

Backport-PR-URL: #19579 PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins · 2018-07-31T18:42:11Z

Backport requested for 8.x in #19244

nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 14, 2017

refack added the wip Issues and PRs that are still a work in progress. label Jun 14, 2017

refack mentioned this pull request Jun 14, 2017

errors, repl: migrate to use internal/errors.js #11347

Closed

4 tasks

jasnell reviewed Jun 14, 2017

View reviewed changes

refack force-pushed the common.expectError-you-would-not-expect-expectError-to-err branch 2 times, most recently from 665dca2 to f8405bc Compare June 14, 2017 23:12

refack changed the title ~~WIP: fix common.expectsError type checking~~ fix common.expectsError type checking Jun 14, 2017

refack removed the wip Issues and PRs that are still a work in progress. label Jun 14, 2017

refack self-assigned this Jun 14, 2017

jasnell reviewed Jun 16, 2017

View reviewed changes

jasnell reviewed Jun 19, 2017

View reviewed changes

BridgeAR closed this Jan 6, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 6, 2018

test: fix wrong error classes passed in as type

f26cabb

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 6, 2018

test: add more asserts to test-internal-errors

15016f2

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 8, 2018

test: add more asserts to test-internal-errors

015c146

PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 9, 2018

test: add more asserts to test-internal-errors

fffb127

PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

targos pushed a commit to targos/node that referenced this pull request Mar 24, 2018

test: add more asserts to test-internal-errors

b59f421

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

targos pushed a commit to targos/node that referenced this pull request Mar 24, 2018

test: fix wrong error classes passed in as type

8e5fd45

PR-URL: nodejs#13686 Fixes: nodejs#13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

targos mentioned this pull request Mar 24, 2018

[v9.x backport] Various http2 changes #19579

Closed

targos pushed a commit that referenced this pull request Mar 30, 2018

test: add more asserts to test-internal-errors

cc68bc2

Backport-PR-URL: #19579 PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

targos pushed a commit that referenced this pull request Mar 30, 2018

test: fix wrong error classes passed in as type

ca97be5

Backport-PR-URL: #19579 PR-URL: #13686 Fixes: #13682 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

targos added backported-to-v9.x and removed backport-requested-v9.x labels Mar 30, 2018

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

refack removed their assignment Oct 12, 2018

fix common.expectsError type checking #13686

fix common.expectsError type checking #13686

Conversation

refack commented Jun 14, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 14, 2017

refack commented Jun 14, 2017

cjihrig commented Jun 14, 2017

jasnell commented Jun 14, 2017

refack commented Jun 14, 2017

refack commented Jun 14, 2017

cjihrig commented Jun 15, 2017

jasnell commented Jun 15, 2017

cjihrig commented Jun 15, 2017

refack commented Jun 16, 2017

jasnell commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

jasnell commented Jun 19, 2017

refack commented Jun 19, 2017

BridgeAR commented Jan 6, 2018

MylesBorins commented Jan 9, 2018

MylesBorins commented Jul 31, 2018

fix `common.expectsError` type checking #13686

fix `common.expectsError` type checking #13686

refack commented Jun 14, 2017 •

edited

Loading

refack Jun 17, 2017 •

edited

Loading

refack commented Jun 19, 2017 •

edited

Loading