Skip to content

Commit

Permalink
tools: prohibit assert.doesNotReject() in Node.js core
Browse files Browse the repository at this point in the history
This makes sure we do not use `assert.doesNotReject()` anywhere in
our code base. This is just a simple wrapper that catches the
rejection and then rejects it again in case of an error. Thus, it is
not useful to do that.

The error message for `assert.doesNotThrow()` is also improved.

PR-URL: nodejs#27402
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
BridgeAR committed Apr 27, 2019
1 parent 38f3526 commit 31b3dd2
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 38 deletions.
6 changes: 5 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,11 @@ module.exports = {
},
{
selector: "CallExpression[callee.property.name='doesNotThrow']",
message: 'Please replace `assert.doesNotThrow()` and add a comment next to the code instead.',
message: 'Do not use `assert.doesNotThrow()`. Write the code without the wrapper and add a comment instead.',
},
{
selector: "CallExpression[callee.property.name='doesNotReject']",
message: 'Do not use `assert.doesNotReject()`. Write the code without the wrapper and add a comment instead.',
},
{
selector: "CallExpression[callee.property.name='rejects'][arguments.length<2]",
Expand Down
2 changes: 2 additions & 0 deletions doc/api/assert.md
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ function. See [`assert.throws()`][] for more details.
Besides the async nature to await the completion behaves identically to
[`assert.doesNotThrow()`][].

<!-- eslint-disable no-restricted-syntax -->
```js
(async () => {
await assert.doesNotReject(
Expand All @@ -465,6 +466,7 @@ Besides the async nature to await the completion behaves identically to
})();
```

<!-- eslint-disable no-restricted-syntax -->
```js
assert.doesNotReject(Promise.reject(new TypeError('Wrong value')))
.then(() => {
Expand Down
68 changes: 34 additions & 34 deletions test/parallel/test-assert-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,36 @@ promises.push(assert.rejects(
}
));

{
const handler = (generated, actual, err) => {
assert.strictEqual(err.generatedMessage, generated);
assert.strictEqual(err.code, 'ERR_ASSERTION');
assert.strictEqual(err.actual, actual);
assert.strictEqual(err.operator, 'rejects');
assert(/rejects/.test(err.stack));
return true;
};
const err = new Error();
promises.push(assert.rejects(
assert.rejects(Promise.reject(null), { code: 'FOO' }),
handler.bind(null, true, null)
));
promises.push(assert.rejects(
assert.rejects(Promise.reject(5), { code: 'FOO' }, 'AAAAA'),
handler.bind(null, false, 5)
));
promises.push(assert.rejects(
assert.rejects(Promise.reject(err), { code: 'FOO' }, 'AAAAA'),
handler.bind(null, false, err)
));
}

// Check `assert.doesNotReject`.
{
// `assert.doesNotReject` accepts a function or a promise
// or a thenable as first argument.
const promise = assert.doesNotReject(() => new Map(), common.mustNotCall());
/* eslint-disable no-restricted-syntax */
let promise = assert.doesNotReject(() => new Map(), common.mustNotCall());
promises.push(assert.rejects(promise, {
message: 'Expected instance of Promise to be returned ' +
'from the "promiseFn" function but got instance of Map.',
Expand Down Expand Up @@ -149,9 +174,7 @@ promises.push(assert.rejects(
code: 'ERR_INVALID_RETURN_VALUE'
})
);
}

{
const handler1 = (err) => {
assert(err instanceof assert.AssertionError,
`${err.name} is not instance of AssertionError`);
Expand All @@ -173,47 +196,24 @@ promises.push(assert.rejects(

const rejectingFn = async () => assert.fail();

let promise = assert.doesNotReject(rejectingFn, common.mustCall(handler1));
promise = assert.doesNotReject(rejectingFn, common.mustCall(handler1));
promises.push(assert.rejects(promise, common.mustCall(handler2)));

promise = assert.doesNotReject(rejectingFn(), common.mustCall(handler1));
promises.push(assert.rejects(promise, common.mustCall(handler2)));

promise = assert.doesNotReject(() => assert.fail(), common.mustNotCall());
promises.push(assert.rejects(promise, common.mustCall(handler1)));
}

promises.push(assert.rejects(
assert.doesNotReject(123),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "promiseFn" argument must be one of type ' +
'Function or Promise. Received type number'
}
));

{
const handler = (generated, actual, err) => {
assert.strictEqual(err.generatedMessage, generated);
assert.strictEqual(err.code, 'ERR_ASSERTION');
assert.strictEqual(err.actual, actual);
assert.strictEqual(err.operator, 'rejects');
assert(/rejects/.test(err.stack));
return true;
};
const err = new Error();
promises.push(assert.rejects(
assert.rejects(Promise.reject(null), { code: 'FOO' }),
handler.bind(null, true, null)
));
promises.push(assert.rejects(
assert.rejects(Promise.reject(5), { code: 'FOO' }, 'AAAAA'),
handler.bind(null, false, 5)
));
promises.push(assert.rejects(
assert.rejects(Promise.reject(err), { code: 'FOO' }, 'AAAAA'),
handler.bind(null, false, err)
assert.doesNotReject(123),
{
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "promiseFn" argument must be one of type ' +
'Function or Promise. Received type number'
}
));
/* eslint-enable no-restricted-syntax */
}

// Make sure all async code gets properly executed.
Expand Down
5 changes: 2 additions & 3 deletions test/parallel/test-fs-readdir-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,12 @@ fs.readdir(readdirDir, {
assertDirents(dirents);
}));

// Check the promisified version
assert.doesNotReject(async () => {
(async () => {
const dirents = await fs.promises.readdir(readdirDir, {
withFileTypes: true
});
assertDirents(dirents);
});
})();

// Check for correct types when the binding returns unknowns
const UNKNOWN = constants.UV_DIRENT_UNKNOWN;
Expand Down

0 comments on commit 31b3dd2

Please sign in to comment.