fix common.expectsError type checking

[refack]

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]

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

[jasnell]

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

[jasnell]

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]

Unrelated whitespace changes

[jasnell]

why change the formatting on these?

[jasnell]

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]

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]

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]

We've given up some things by using custom errors

Like what?

[refack]

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

1. using constructor.name I found some bugs in /parallel/test-internal-errors.js
2. 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)
3. found two places that used it for "regular" errors: parallel/test-fs-realpath.js & parallel/test-http-request-invalid-method-error.js

[refack]

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

[cjihrig]

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]

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]

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]

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]

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]

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

[refack]

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

[jasnell]

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

[refack]

Ack

[jasnell]

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

[refack]

Because

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

[jasnell]

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

[cjihrig]

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

[refack]

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

[jasnell]

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.

[cjihrig]

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?

[refack]

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]

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

1. ceef6c7 - expectsError({type}) checks type name, so expectError({type: Error})(new TypeError()) will fail
2. 2012d3f - fix 2 bugs in test-internal-errors.js
   b0c8375 - and add more assertions to that file
3. 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)
4. f8405bc - Fix 2 case that violate (3)

/cc @nodejs/testing

[jasnell]

Unnecessary whitespace change?

[jasnell]

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

[refack]

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

[BridgeAR]

Landed in 15016f2, 29d99c1, f26cabb

[MylesBorins]

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

[MylesBorins]

Backport requested for 8.x in #19244