Skip to content

assert: improve assert rejects #19885

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.

Sign up for GitHub

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

Closed
BridgeAR wants to merge 4 commits into from
Closed

assert: improve assert rejects #19885

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ac3559
assert: add direct promises support in rejects
BridgeAR Apr 8, 2018
7ba5d61
tools: add eslintrc rule for `assert.rejects`
BridgeAR Apr 8, 2018
fa6ac5f
doc: improve assert documentation
BridgeAR Apr 8, 2018
0e6e876
fixup: address comments
BridgeAR Apr 13, 2018
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: 5 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,13 @@ module.exports = {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']",
message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead."
},
{
selector: `CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]`,
message: 'assert.rejects() must be invoked with at least two arguments.',
},
{
selector: `CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])`,
message: 'use a regular expression for second argument of assert.throws()',
message: 'Use an object as second argument of assert.throws()',
},
{
selector: `CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]`,
Expand Down
93 changes: 61 additions & 32 deletions doc/api/assert.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,21 +378,24 @@ parameter is an instance of an [`Error`][] then it will be thrown instead of the
<!-- YAML
added: REPLACEME
-->
* `block` {Function}
* `block` {Function|Promise}
* `error` {RegExp|Function}
* `message` {any}

Awaits for the promise returned by function `block` to complete and not be
rejected.
Awaits the `block` promise or, if `block` is a function, immediately call the
Copy link
Member

@trivikr trivikr Apr 14, 2018

Choose a reason for hiding this comment

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

nit: immediately calls the function and awaits the returned promise
Ditto on line 927

function and await the returned promise to complete. It will then check that the
promise is not rejected.

If `block` is a function and it throws synchronously, that error will be
rejected directly without checking the `error` handler.

Please note: Using `assert.doesNotReject()` is actually not useful because there
is little benefit by catching a rejection and then rejecting it again. Instead,
consider adding a comment next to the specific code path that should not reject
and keep error messages as expressive as possible.

When `assert.doesNotReject()` is called, it will immediately call the `block`
function, and awaits for completion. See [`assert.rejects()`][] for more
details.
If specified, `error` can be a [`Class`][], [`RegExp`][] or a validation
function. See [`assert.throws()`][] for more details.

Besides the async nature to await the completion behaves identically to
[`assert.doesNotThrow()`][].
Expand All @@ -409,12 +412,10 @@ Besides the async nature to await the completion behaves identically to
```

```js
assert.doesNotReject(
() => Promise.reject(new TypeError('Wrong value')),
SyntaxError
).then(() => {
// ...
});
assert.doesNotReject(Promise.reject(new TypeError('Wrong value')))
.then(() => {
// ...
});
```

## assert.doesNotThrow(block[, error][, message])
Expand All @@ -432,8 +433,7 @@ changes:
* `error` {RegExp|Function}
* `message` {any}

Asserts that the function `block` does not throw an error. See
[`assert.throws()`][] for more details.
Asserts that the function `block` does not throw an error.

Please note: Using `assert.doesNotThrow()` is actually not useful because there
is no benefit by catching an error and then rethrowing it. Instead, consider
Expand All @@ -448,6 +448,9 @@ parameter, then an `AssertionError` is thrown. If the error is of a different
type, or if the `error` parameter is undefined, the error is propagated back
to the caller.

If specified, `error` can be a [`Class`][], [`RegExp`][] or a validation
function. See [`assert.throws()`][] for more details.

The following, for instance, will throw the [`TypeError`][] because there is no
matching error type in the assertion:

Expand Down Expand Up @@ -484,7 +487,7 @@ assert.doesNotThrow(
() => {
throw new TypeError('Wrong value');
},
TypeError,
/Wrong value/,
'Whoops'
);
// Throws: AssertionError: Got unwanted exception (TypeError). Whoops
Expand Down Expand Up @@ -916,20 +919,24 @@ assert(0);
<!-- YAML
added: REPLACEME
-->
* `block` {Function}
* `error` {RegExp|Function|Object}
* `block` {Function|Promise}
* `error` {RegExp|Function|Object|Error}
* `message` {any}

Awaits for promise returned by function `block` to be rejected.
Awaits the `block` promise or, if `block` is a function, immediately call the
function and await the returned promise to complete. It will then check that the
promise is rejected.

When `assert.rejects()` is called, it will immediately call the `block`
function, and awaits for completion.
If `block` is a function and it throws synchronously, that error will be
rejected directly without checking the `error` handler.
Copy link
Member

@mcollina mcollina Apr 10, 2018

Choose a reason for hiding this comment

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

what do you mean with this?

Copy link
Member Author

@BridgeAR BridgeAR Apr 10, 2018

Choose a reason for hiding this comment

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

Synchronously thrown errors (due to the block() call) will directly cause a rejected promise to be returned (with that error). That is different in the way that a rejection caused by calling block() will trigger the potential error handler (second argument). If the rejected error does not adhere to the error handler, then it will cause a rejection. Otherwise not.

I do see that this is not well worded yet and it would be great to get a suggestion for improvement. Looping in @nodejs/documentation

Copy link
Member Author

@BridgeAR BridgeAR Apr 12, 2018

Choose a reason for hiding this comment

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

Ping @vsemozhetbyt @Trott

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 12, 2018

Choose a reason for hiding this comment

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

I cannot think of anything more clear(

Copy link
Contributor

@not-an-aardvark not-an-aardvark Apr 12, 2018

Choose a reason for hiding this comment

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

Maybe something like this?

If block is a function and it throws an error synchronously, assert.rejects will return a rejected Promise with that error without checking the error handler.


Besides the async nature to await the completion behaves identically to
[`assert.throws()`][].

If specified, `error` can be a constructor, [`RegExp`][], a validation
function, or an object where each property will be tested for.
If specified, `error` can be a [`Class`][], [`RegExp`][], a validation function,
an object where each property will be tested for or an instance of error where
Copy link
Member

@trivikr trivikr Apr 14, 2018

Choose a reason for hiding this comment

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

nit: an object where each property will be tested for, or an instance
Ditto on line 1027

each property will be tested for including the non-enumerable `message` and
`name` properties.

If specified, `message` will be the message provided by the `AssertionError` if
the block fails to reject.
Expand All @@ -938,22 +945,31 @@ the block fails to reject.
(async () => {
await assert.rejects(
async () => {
throw new Error('Wrong value');
throw new TypeError('Wrong value');
},
Error
{
name: 'TypeError',
message: 'Wrong value'
}
);
})();
```

```js
assert.rejects(
() => Promise.reject(new Error('Wrong value')),
Promise.reject(new Error('Wrong value')),
Error
).then(() => {
// ...
});
```

Note that `error` cannot be a string. If a string is provided as the second
argument, then `error` is assumed to be omitted and the string will be used for
`message` instead. This can lead to easy-to-miss mistakes. Please read the
example in [`assert.throws()`][] carefully if using a string as the second
argument gets considered.

## assert.strictEqual(actual, expected[, message])
<!-- YAML
added: v0.1.21
Expand Down Expand Up @@ -1000,13 +1016,15 @@ changes:
description: The `error` parameter can now be an arrow function.
-->
* `block` {Function}
* `error` {RegExp|Function|Object}
* `error` {RegExp|Function|Object|Error}
* `message` {any}

Expects the function `block` to throw an error.

If specified, `error` can be a constructor, [`RegExp`][], a validation
function, or an object where each property will be tested for.
If specified, `error` can be a [`Class`][], [`RegExp`][], a validation function,
an object where each property will be tested for or an instance of error where
each property will be tested for including the non-enumerable `message` and
`name` properties.

If specified, `message` will be the message provided by the `AssertionError` if
the block fails to throw.
Expand Down Expand Up @@ -1055,10 +1073,11 @@ assert.throws(
Custom error object / error instance:

```js
const err = new TypeError('Wrong value');
err.code = 404;

assert.throws(
() => {
const err = new TypeError('Wrong value');
err.code = 404;
throw err;
},
{
Expand All @@ -1067,9 +1086,19 @@ assert.throws(
// Note that only properties on the error object will be tested!
}
);

// Fails due to the different `message` and `name` properties:
assert.throws(
() => {
const otherErr = new Error('Not found');
otherErr.code = 404;
throw otherErr;
},
err // This tests for `message`, `name` and `code`.
);
```

Note that `error` can not be a string. If a string is provided as the second
Note that `error` cannot be a string. If a string is provided as the second
argument, then `error` is assumed to be omitted and the string will be used for
`message` instead. This can lead to easy-to-miss mistakes. Please read the
example below carefully if using a string as the second argument gets
Expand Down Expand Up @@ -1107,6 +1136,7 @@ assert.throws(throwingFirst, /Second$/);
Due to the confusing notation, it is recommended not to use a string as the
second argument. This might lead to difficult-to-spot errors.

[`Class`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes
[`Error.captureStackTrace`]: errors.html#errors_error_capturestacktrace_targetobject_constructoropt
[`Error`]: errors.html#errors_class_error
[`Map`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map
Expand All @@ -1123,7 +1153,6 @@ second argument. This might lead to difficult-to-spot errors.
[`assert.notDeepStrictEqual()`]: #assert_assert_notdeepstrictequal_actual_expected_message
[`assert.notStrictEqual()`]: #assert_assert_notstrictequal_actual_expected_message
[`assert.ok()`]: #assert_assert_ok_value_message
[`assert.rejects()`]: #assert_assert_rejects_block_error_message
[`assert.strictEqual()`]: #assert_assert_strictequal_actual_expected_message
[`assert.throws()`]: #assert_assert_throws_block_error_message
[`strict mode`]: #assert_strict_mode
Expand Down
4 changes: 3 additions & 1 deletion lib/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ rules:
- error
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']"
message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead."
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]"
message: "assert.rejects() must be invoked with at least two arguments."
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
message: "use a regular expression for second argument of assert.throws()"
message: "Use an object as second argument of assert.throws()"
- selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]"
message: "assert.throws() must be invoked with at least two arguments."
- selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
Expand Down
50 changes: 27 additions & 23 deletions lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const {
}
} = require('internal/errors');
const { openSync, closeSync, readSync } = require('fs');
const { inspect } = require('util');
const { inspect, types: { isPromise } } = require('util');
const { EOL } = require('os');
const { NativeModule } = require('internal/bootstrap/loaders');

Expand Down Expand Up @@ -448,13 +448,24 @@ function getActual(block) {
return NO_EXCEPTION_SENTINEL;
}

function checkIsPromise(obj) {
return isPromise(obj) ||
Copy link
Contributor

@not-an-aardvark not-an-aardvark Apr 10, 2018

Choose a reason for hiding this comment

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

I would recommend just checking for the presence of a .then function here, without doing any of the other checks. That would make it compatible with Promise libraries and the Promise.resolve method and Promise libraries, as well as the mental model of "anything that can be await-ed to get a different value".

Copy link
Member Author

@BridgeAR BridgeAR Apr 10, 2018

Choose a reason for hiding this comment

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

My intention was not to support Promises/A+ but ES6 promises. And if I read the spec correct, the check is fine as I implemented it. Only checking for obj !== null && typeof obj.then === 'function' would allow multiple false positives, so I would rather stick to the current check.

I can remove the isPromise check though as any real promise will also adhere to the later checks.

Copy link
Member

@targos targos Apr 12, 2018

Choose a reason for hiding this comment

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

If you only want to support ES6 promises, why do you need the checks below?

Copy link
Member Author

@BridgeAR BridgeAR Apr 12, 2018

Choose a reason for hiding this comment

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

Well, I do not only want to support native promises. Every implementation that is spec compliant should be supported out of my perspective. That is not the case for all thenables. I do not have a strong opinion on this though, so I can go for what ever is suggested. Only having the upper check is definitely not enough though. The very least that should also be checked for is typeof obj === 'object' || typeof obj === 'function and I would argue also `typeof obj.catch === 'function'. In that case we should be relatively safe.

The difference with that check to the current implementation would be to also accept thenables that use a function instead a object as Promise.

Copy link
Member

@targos targos Apr 12, 2018

Choose a reason for hiding this comment

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

I'm not against the current check, but it would be nice to have a small comment explaining this in the code.

Copy link
Contributor

@not-an-aardvark not-an-aardvark Apr 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There are two categories of "promise-like" objects that have special behavior in the language: native promises and thenables. By doing extra checks (e.g. for a .catch method), we would effectively be adding a third category (the categories would be "native promises", "thenables", and "thenables which also have a .catch method") This seems like it would complicate the model by adding an arbitrary additional check, and it's not clear to me that there is a substantial benefit to adding a .catch check. So in my opinion, it would be slightly better to just detect thenables for API simplicity.

obj !== null && typeof obj === 'object' &&
typeof obj.then === 'function' &&
typeof obj.catch === 'function';
}

async function waitForActual(block) {
if (typeof block !== 'function') {
throw new ERR_INVALID_ARG_TYPE('block', 'Function', block);
let resultPromise;
if (typeof block === 'function') {
// Return a rejected promise if `block` throws synchronously.
resultPromise = block();
Copy link
Member

@targos targos Apr 12, 2018

Choose a reason for hiding this comment

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

Should we check that block() effectively returns a Promise?

Copy link
Member Author

@BridgeAR BridgeAR Apr 12, 2018

Choose a reason for hiding this comment

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

I have a follow up PR in #19886 that does exactly that :-)

} else if (checkIsPromise(block)) {
resultPromise = block;
} else {
throw new ERR_INVALID_ARG_TYPE('block', ['Function', 'Promise'], block);
}

// Return a rejected promise if `block` throws synchronously.
const resultPromise = block();
try {
await resultPromise;
} catch (e) {
Expand All @@ -480,7 +491,7 @@ function expectsError(stackStartFn, actual, error, message) {
details += ` (${error.name})`;
}
details += message ? `: ${message}` : '.';
const fnType = stackStartFn === rejects ? 'rejection' : 'exception';
const fnType = stackStartFn.name === 'rejects' ? 'rejection' : 'exception';
innerFail({
actual,
expected: error,
Expand All @@ -505,7 +516,8 @@ function expectsNoError(stackStartFn, actual, error, message) {

if (!error || expectedException(actual, error)) {
const details = message ? `: ${message}` : '.';
const fnType = stackStartFn === doesNotReject ? 'rejection' : 'exception';
const fnType = stackStartFn.name === 'doesNotReject' ?
'rejection' : 'exception';
innerFail({
actual,
expected: error,
Expand All @@ -518,29 +530,21 @@ function expectsNoError(stackStartFn, actual, error, message) {
throw actual;
}

function throws(block, ...args) {
assert.throws = function throws(block, ...args) {
expectsError(throws, getActual(block), ...args);
}

assert.throws = throws;
};

async function rejects(block, ...args) {
assert.rejects = async function rejects(block, ...args) {
expectsError(rejects, await waitForActual(block), ...args);
}

assert.rejects = rejects;
};

function doesNotThrow(block, ...args) {
assert.doesNotThrow = function doesNotThrow(block, ...args) {
expectsNoError(doesNotThrow, getActual(block), ...args);
}

assert.doesNotThrow = doesNotThrow;
};

async function doesNotReject(block, ...args) {
assert.doesNotReject = async function doesNotReject(block, ...args) {
expectsNoError(doesNotReject, await waitForActual(block), ...args);
}

assert.doesNotReject = doesNotReject;
};

assert.ifError = function ifError(err) {
if (err !== null && err !== undefined) {
Expand Down
Loading