From 31b3dd28429df7ea7ebc84bdfaf8d9eb9e417b41 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 25 Apr 2019 00:46:40 +0200 Subject: [PATCH] tools: prohibit `assert.doesNotReject()` in Node.js core 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: https://github.com/nodejs/node/pull/27402 Reviewed-By: Rich Trott Reviewed-By: Benjamin Gruenbaum Reviewed-By: Beth Griggs Reviewed-By: James M Snell --- .eslintrc.js | 6 ++- doc/api/assert.md | 2 + test/parallel/test-assert-async.js | 68 +++++++++++++------------- test/parallel/test-fs-readdir-types.js | 5 +- 4 files changed, 43 insertions(+), 38 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 9aa90c6366232d..cfada1f65c181e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -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]", diff --git a/doc/api/assert.md b/doc/api/assert.md index 9028d21f0d9df3..0a7d35f105e48e 100644 --- a/doc/api/assert.md +++ b/doc/api/assert.md @@ -454,6 +454,7 @@ function. See [`assert.throws()`][] for more details. Besides the async nature to await the completion behaves identically to [`assert.doesNotThrow()`][]. + ```js (async () => { await assert.doesNotReject( @@ -465,6 +466,7 @@ Besides the async nature to await the completion behaves identically to })(); ``` + ```js assert.doesNotReject(Promise.reject(new TypeError('Wrong value'))) .then(() => { diff --git a/test/parallel/test-assert-async.js b/test/parallel/test-assert-async.js index 140bd05d1a3c8f..8aad1d865c097e 100644 --- a/test/parallel/test-assert-async.js +++ b/test/parallel/test-assert-async.js @@ -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.', @@ -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`); @@ -173,7 +196,7 @@ 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)); @@ -181,39 +204,16 @@ promises.push(assert.rejects( 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. diff --git a/test/parallel/test-fs-readdir-types.js b/test/parallel/test-fs-readdir-types.js index 96a3b73098728d..78f1b0d4e1b627 100644 --- a/test/parallel/test-fs-readdir-types.js +++ b/test/parallel/test-fs-readdir-types.js @@ -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;