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

assert: adds rejects() and doesNotReject() #18023

Closed
wants to merge 1 commit 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
84 changes: 84 additions & 0 deletions doc/api/assert.md
Expand Up @@ -312,6 +312,45 @@ parameter is undefined, a default error message is assigned. If the `message`
parameter is an instance of an [`Error`][] then it will be thrown instead of the
`AssertionError`.

## assert.doesNotReject(block[, error][, message])
<!-- YAML
added: REPLACEME
-->
* `block` {Function}
* `error` {RegExp|Function}
* `message` {any}

> Stability: 1 - Experimental

Awaits for the promise returned by function `block` to complete and not be
rejected. See [`assert.rejects()`][] for more details.

When `assert.doesNotReject()` is called, it will immediately call the `block`
function, and awaits for completion.

Besides the async nature to await the completion it behaves identical to
[`assert.doesNotThrow()`][].

```js
(async () => {
await assert.doesNotReject(
async () => {
throw new TypeError('Wrong value');
},
SyntaxError
);
})();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This async wrapper is really ugly.
But If I omit it, the linter cannot even parse this code (top-level await is not for tomorrow...).

The other option would be to remove await, but it doesn't highlight the async nature of doesNotReject().
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine to keep it as is.

```

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

## assert.doesNotThrow(block[, error][, message])
<!-- YAML
added: v0.1.21
Expand Down Expand Up @@ -836,6 +875,50 @@ If the values are not strictly equal, an `AssertionError` is thrown with a
`message` parameter is an instance of an [`Error`][] then it will be thrown
instead of the `AssertionError`.

## assert.rejects(block[, error][, message])
<!-- YAML
added: REPLACEME
-->
* `block` {Function}
* `error` {RegExp|Function|Object}
* `message` {any}

> Stability: 1 - Experimental

Awaits for promise returned by function `block` to be rejected.

When `assert.rejects()` is called, it will immediately call the `block`
function, and awaits for completion.

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

If specified, `message` will be the message provided by the `AssertionError` if
the block fails to throw.

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

```js
(async () => {
await assert.rejects(
async () => {
throw new Error('Wrong value');
},
Error
);
})();
```

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

## assert.throws(block[, error][, message])
<!-- YAML
added: v0.1.21
Expand Down Expand Up @@ -972,6 +1055,7 @@ second argument. This might lead to difficult-to-spot errors.
[`assert.ok()`]: #assert_assert_ok_value_message
[`assert.strictEqual()`]: #assert_assert_strictequal_actual_expected_message
[`assert.throws()`]: #assert_assert_throws_block_error_message
[`assert.rejects()`]: #assert_assert_rejects_block_error_message
[`strict mode`]: #assert_strict_mode
[Abstract Equality Comparison]: https://tc39.github.io/ecma262/#sec-abstract-equality-comparison
[Object.prototype.toString()]: https://tc39.github.io/ecma262/#sec-object.prototype.tostring
Expand Down
64 changes: 49 additions & 15 deletions lib/assert.js
Expand Up @@ -425,14 +425,23 @@ function getActual(block) {
return NO_EXCEPTION_SENTINEL;
}

// Expected to throw an error.
assert.throws = function throws(block, error, message) {
const actual = getActual(block);
async function waitForActual(block) {
if (typeof block !== 'function') {
throw new ERR_INVALID_ARG_TYPE('block', 'Function', block);
}
try {
await block();
} catch (e) {
return e;
}
return NO_EXCEPTION_SENTINEL;
}

function expectsError(stackStartFn, actual, error, message) {
if (typeof error === 'string') {
if (arguments.length === 3)
if (arguments.length === 4) {
throw new ERR_INVALID_ARG_TYPE('error', ['Function', 'RegExp'], error);

}
message = error;
error = null;
}
Expand All @@ -443,21 +452,21 @@ assert.throws = function throws(block, error, message) {
details += ` (${error.name})`;
}
details += message ? `: ${message}` : '.';
const fnType = stackStartFn === rejects ? 'rejection' : 'exception';
innerFail({
actual,
expected: error,
operator: 'throws',
message: `Missing expected exception${details}`,
stackStartFn: throws
operator: stackStartFn.name,
message: `Missing expected ${fnType}${details}`,
stackStartFn
});
}
if (error && expectedException(actual, error, message) === false) {
throw actual;
}
};
}

assert.doesNotThrow = function doesNotThrow(block, error, message) {
const actual = getActual(block);
function expectsNoError(stackStartFn, actual, error, message) {
if (actual === NO_EXCEPTION_SENTINEL)
return;

Expand All @@ -468,16 +477,41 @@ assert.doesNotThrow = function doesNotThrow(block, error, message) {

if (!error || expectedException(actual, error)) {
const details = message ? `: ${message}` : '.';
const fnType = stackStartFn === doesNotReject ? 'rejection' : 'exception';
innerFail({
actual,
expected: error,
operator: 'doesNotThrow',
message: `Got unwanted exception${details}\n${actual && actual.message}`,
stackStartFn: doesNotThrow
operator: stackStartFn.name,
message: `Got unwanted ${fnType}${details}\n${actual && actual.message}`,
stackStartFn
});
}
throw actual;
};
}

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

assert.throws = throws;
Copy link
Member

Choose a reason for hiding this comment

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

Nit (non-blocking): I personally prefer the following to save lines:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also prefer function expressions.
But even named, function expression do not benefit from hoisting.

This would imply to use assert.doesNotReject() here, and here.

Same goes for other functions.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. We could check for the function name instead but that could theoretically be manipulated.

So just keep it as is. Thanks for the heads up.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, while looking at the code again: we actually already use the function name and do not strictly test if it it a "valid" function. So we could indeed just switch to name checking only.


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

assert.rejects = rejects;

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

assert.doesNotThrow = doesNotThrow;

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
66 changes: 66 additions & 0 deletions test/parallel/test-assert-async.js
@@ -0,0 +1,66 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { promisify } = require('util');
const wait = promisify(setTimeout);

/* eslint-disable prefer-common-expectserror, no-restricted-properties */

// Test assert.rejects() and assert.doesNotReject() by checking their
// expected output and by verifying that they do not work sync

assert.rejects(
() => assert.fail(),
common.expectsError({
code: 'ERR_ASSERTION',
type: assert.AssertionError,
message: 'Failed'
})
);

assert.doesNotReject(() => {});

{
const promise = assert.rejects(async () => {
await wait(1);
assert.fail();
}, common.expectsError({
code: 'ERR_ASSERTION',
type: assert.AssertionError,
message: 'Failed'
}));
assert.doesNotReject(() => promise);
}

{
const promise = assert.doesNotReject(async () => {
await wait(1);
throw new Error();
});
assert.rejects(() => promise,
(err) => {
assert(err instanceof assert.AssertionError,
`${err.name} is not instance of AssertionError`);
assert.strictEqual(err.code, 'ERR_ASSERTION');
assert(/^Got unwanted rejection\.\n$/.test(err.message));
assert.strictEqual(err.operator, 'doesNotReject');
assert.ok(!err.stack.includes('at Function.doesNotReject'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted rejection.

    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsNoError (assert.js:464:5)
    at Function.doesNotReject (assert.js:494:3)
    at <anonymous>

With cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted rejection.

    at <anonymous>

return true;
}
);
}

{
const promise = assert.rejects(() => {});
assert.rejects(() => promise,
(err) => {
assert(err instanceof assert.AssertionError,
`${err.name} is not instance of AssertionError`);
assert.strictEqual(err.code, 'ERR_ASSERTION');
assert(/^Missing expected rejection\.$/.test(err.message));
assert.strictEqual(err.operator, 'rejects');
assert.ok(!err.stack.includes('at Function.rejects'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Missing expected rejection.
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsError (assert.js:439:5)
    at Function.rejects (assert.js:482:3)
    at <anonymous>

With cleaning:

AssertionError [ERR_ASSERTION]: Missing expected rejection.
    at <anonymous>

return true;
}
);
}
11 changes: 11 additions & 0 deletions test/parallel/test-assert.js
Expand Up @@ -116,6 +116,7 @@ assert.throws(() => thrower(TypeError));
} catch (e) {
threw = true;
assert.ok(e instanceof a.AssertionError);
assert.ok(!e.stack.includes('at Function.doesNotThrow'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested, I removed the unnecessary test, and enriched an existing one.

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted exception.
[object Object]
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsNoError (assert.js:464:5)
    at Function.doesNotThrow (assert.js:488:3)
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:450:7)

With cleaning:

AssertionError [ERR_ASSERTION]: Got unwanted exception.
[object Object]
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:446:7)

}
assert.ok(threw, 'a.doesNotThrow is not catching type matching errors');
}
Expand Down Expand Up @@ -221,6 +222,16 @@ a.throws(() => thrower(TypeError), (err) => {
code: 'ERR_ASSERTION',
message: /^Missing expected exception \(TypeError\): fhqwhgads$/
}));

let threw = false;
try {
a.throws(noop);
} catch (e) {
threw = true;
assert.ok(e instanceof a.AssertionError);
assert.ok(!e.stack.includes('at Function.throws'));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As requested, I reverted the existing test.
But I had to add this new one, because:

  • only 4 tests are triggering the 'Missing expected exception' which I'm looking for
  • those tests are using common.expectsError( and the same assert.throws(() => { a.throws( pattern which overcomplicates the stack trace.

Without stack trace cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at new AssertionError (internal/errors.js:320:11)
    at innerFail (assert.js:74:9)
    at expectsError (assert.js:439:5)
    at Function.throws (assert.js:476:3)
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)

With cleaning:

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at Object.<anonymous> (D:\Workspaces\perso\node\test\parallel\test-assert.js:566:12)

}
assert.ok(threw);
}

const circular = { y: 1 };
Expand Down