Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: throw a special error in internal/assert #26635

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1148,6 +1148,12 @@ is set for the `Http2Stream`.
`http2.connect()` was passed a URL that uses any protocol other than `http:` or
`https:`.

<a id="ERR_INTERNAL_ASSERTION"></a>
### ERR_INTERNAL_ASSERTION

There was a bug in Node.js or incorrect usage of Node.js internals.
To fix the error, open an issue at https://github.com/nodejs/node/issues.

<a id="ERR_INCOMPATIBLE_OPTION_PAIR"></a>
### ERR_INCOMPATIBLE_OPTION_PAIR

Expand Down
13 changes: 11 additions & 2 deletions lib/internal/assert.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
'use strict';

let error;
function lazyError() {
if (!error) {
error = require('internal/errors').codes.ERR_INTERNAL_ASSERTION;
}
return error;
}
function assert(value, message) {
if (!value) {
require('assert')(value, message);
const ERR_INTERNAL_ASSERTION = lazyError();
throw new ERR_INTERNAL_ASSERTION(message);
}
}

function fail(message) {
require('assert').fail(message);
const ERR_INTERNAL_ASSERTION = lazyError();
throw new ERR_INTERNAL_ASSERTION(message);
}

assert.fail = fail;
Expand Down
7 changes: 7 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,13 @@ E('ERR_INSPECTOR_CLOSED', 'Session was closed', Error);
E('ERR_INSPECTOR_COMMAND', 'Inspector error %d: %s', Error);
E('ERR_INSPECTOR_NOT_AVAILABLE', 'Inspector is not available', Error);
E('ERR_INSPECTOR_NOT_CONNECTED', 'Session is not connected', Error);
E('ERR_INTERNAL_ASSERTION', (message) => {
const suffix = 'This is caused by either a bug in Node.js ' +
'or incorrect usage of Node.js internals.\n' +
'Please open an issue with this stack trace at ' +
'https://github.com/nodejs/node/issues\n';
return message === undefined ? suffix : `${message}\n${suffix}`;
}, Error);
E('ERR_INVALID_ADDRESS_FAMILY', function(addressType, host, port) {
this.host = host;
this.port = port;
Expand Down
14 changes: 14 additions & 0 deletions test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,19 @@ function expectsError(fn, settings, exact) {
return mustCall(innerFn, exact);
}

const suffix = 'This is caused by either a bug in Node.js ' +
'or incorrect usage of Node.js internals.\n' +
'Please open an issue with this stack trace at ' +
'https://github.com/nodejs/node/issues\n';

function expectsInternalAssertion(fn, message) {
assert.throws(fn, {
message: `${message}\n${suffix}`,
name: 'Error',
code: 'ERR_INTERNAL_ASSERTION'
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not add this helper function in common. Common is AFAIC pretty overboarded.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's where common.expectsError is...I guess we could move these and other expectsSomething to one file when the time comes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, surprisingly, test/common.js is even less than 1000 lines? That's even smaller than a lot of our tests :S

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about having a lot of lines but this file is loaded for every single test file and I would rather completely get rid of something that we always use.

Having something more specific would be fine but I actually doubt that this will often be tested for and currently there are only for places in which this helper would be used in.

About common.expectsError() it is already used significantly less as we can always use assert.throws() instead. Only callbacks should always use common.expectsError().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you strongly prefer the helper function? To me it still does not seem necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the consensus is you don't need to validate the message of errors in more then one place (in this case test/message/internal_assert.js is enough), so there's no need for the value that you are DRYing in the first place.

Copy link
Member Author

@joyeecheung joyeecheung Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack This is special in that the message is what we want to test for in the test cases, because the code is constant across all internal assertions and we intentionally do not give error codes for assertions coming out of completely different type of bugs.

For instance, we want to verify that we tell the user handle must be a FSEvent if the user monkey patches watcher._handle incorrectly, but we do not want to create a code for that special type of bugs because the user is simply violating the API contract and it gives us little to classify all these violations and assign permanent error codes instead of just telling them no. However it's still useful to verify that we hint something helpful in the message.

Copy link
Member Author

@joyeecheung joyeecheung Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to make sure we provide useful information in the assertions, even for our own sake - just making sure that we say "you are hitting a bug, please open an issue" is not enough, see all the useless error messages in #26798 before #26859 (though that one is not verifiable in user land).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is special in that the message is what we want to test for in the test cases,

What I mean there's no need to validate the message formatting mechanism more then once.
For example you can store the specific prefix behind a Symbol field and validate just it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern about common/index is only about the requirement of a common helper function. I would love to remove the requirement for our test files at some point.

I already gave my LG, so from my side this can land as is.


function skipIfInspectorDisabled() {
if (!process.features.inspector) {
skip('V8 inspector is disabled');
Expand Down Expand Up @@ -729,6 +742,7 @@ module.exports = {
enoughTestCpu,
enoughTestMem,
expectsError,
expectsInternalAssertion,
expectWarning,
getArrayBufferViews,
getBufferSources,
Expand Down
7 changes: 7 additions & 0 deletions test/message/internal_assert.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

// Flags: --expose-internals
require('../common');

const assert = require('internal/assert');
assert(false);
15 changes: 15 additions & 0 deletions test/message/internal_assert.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
internal/assert.js:*
throw new ERR_INTERNAL_ASSERTION(message);
^

Error [ERR_INTERNAL_ASSERTION]: This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

at assert (internal/assert.js:*:*)
at * (*test*message*internal_assert.js:7:1)
at *
at *
at *
at *
at *
at *
7 changes: 7 additions & 0 deletions test/message/internal_assert_fail.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
'use strict';

// Flags: --expose-internals
require('../common');

const assert = require('internal/assert');
assert.fail('Unreachable!');
16 changes: 16 additions & 0 deletions test/message/internal_assert_fail.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
internal/assert.js:*
throw new ERR_INTERNAL_ASSERTION(message);
^

Error [ERR_INTERNAL_ASSERTION]: Unreachable!
This is caused by either a bug in Node.js or incorrect usage of Node.js internals.
Please open an issue with this stack trace at https://github.com/nodejs/node/issues

at Function.fail (internal/assert.js:*:*)
at * (*test*message*internal_assert_fail.js:7:8)
at *
at *
at *
at *
at *
at *
10 changes: 3 additions & 7 deletions test/parallel/test-internal-assert.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
// Flags: --expose-internals
'use strict';

// This tests that the internal assert module works as expected.
// The failures are tested in test/message.

require('../common');

const assert = require('assert');
const internalAssert = require('internal/assert');

// Should not throw.
internalAssert(true);
internalAssert(true, 'fhqwhgads');

assert.throws(() => { internalAssert(false); }, assert.AssertionError);
assert.throws(() => { internalAssert(false, 'fhqwhgads'); },
{ code: 'ERR_ASSERTION', message: 'fhqwhgads' });
assert.throws(() => { internalAssert.fail('fhqwhgads'); },
{ code: 'ERR_ASSERTION', message: 'fhqwhgads' });
8 changes: 3 additions & 5 deletions test/parallel/test-internal-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,10 @@ errors.E('TEST_ERROR_2', (a, b) => `${a} ${b}`, Error);
}

{
assert.throws(
common.expectsInternalAssertion(
() => new errors.codes.TEST_ERROR_1(),
{
message: 'Code: TEST_ERROR_1; The provided arguments ' +
'length (0) does not match the required ones (1).'
}
'Code: TEST_ERROR_1; The provided arguments ' +
'length (0) does not match the required ones (1).'
);
}

Expand Down
7 changes: 2 additions & 5 deletions test/parallel/test-tls-basic-validations.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,9 @@ common.expectsError(
assert.throws(() => tls.createServer({ ticketKeys: Buffer.alloc(0) }),
/TypeError: Ticket keys length must be 48 bytes/);

common.expectsError(
common.expectsInternalAssertion(
() => tls.createSecurePair({}),
{
code: 'ERR_ASSERTION',
message: 'context.context must be a NativeSecureContext'
}
'context.context must be a NativeSecureContext'
);

{
Expand Down
18 changes: 9 additions & 9 deletions test/sequential/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,14 +115,14 @@ tmpdir.refresh();
// https://github.com/joyent/node/issues/6690
{
let oldhandle;
assert.throws(() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
w._handle = { close: w._handle.close };
w.close();
}, {
message: 'handle must be a FSEvent',
code: 'ERR_ASSERTION'
});
common.expectsInternalAssertion(
() => {
const w = fs.watch(__filename, common.mustNotCall());
oldhandle = w._handle;
w._handle = { close: w._handle.close };
w.close();
},
'handle must be a FSEvent'
);
oldhandle.close(); // clean up
}