From a5486900a61007f11ff1500d9d1770be3d685f52 Mon Sep 17 00:00:00 2001 From: Andreas Lind Date: Tue, 5 Mar 2019 14:30:57 +0100 Subject: [PATCH 1/5] Implement no-return-from-async rule --- docs/rules/README.md | 1 + docs/rules/no-return-from-async.md | 44 ++++++++++ index.js | 1 + lib/rules/no-return-from-async.js | 69 +++++++++++++++ test/rules/no-return-from-async.js | 135 +++++++++++++++++++++++++++++ 5 files changed, 250 insertions(+) create mode 100644 docs/rules/no-return-from-async.md create mode 100644 lib/rules/no-return-from-async.js create mode 100644 test/rules/no-return-from-async.js diff --git a/docs/rules/README.md b/docs/rules/README.md index 2bfd26d..2c7ba16 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -11,6 +11,7 @@ * [no-nested-tests](no-nested-tests.md) - disallow tests to be nested within other tests * [no-pending-tests](no-pending-tests.md) - disallow pending/unimplemented mocha tests * [no-return-and-callback](no-return-and-callback.md) - disallow returning in a test or hook function that uses a callback +* [no-return-from-async](no-return-from-async.md) - disallow returning from an async test or hook * [no-setup-in-describe](no-setup-in-describe.md) - disallow calling functions and dot operators directly in describe blocks * [no-sibling-hooks](no-sibling-hooks.md) - disallow duplicate uses of a hook at the same level inside a describe * [no-skipped-tests](no-skipped-tests.md) - disallow skipped mocha tests (fixable) diff --git a/docs/rules/no-return-from-async.md b/docs/rules/no-return-from-async.md new file mode 100644 index 0000000..da63bc8 --- /dev/null +++ b/docs/rules/no-return-from-async.md @@ -0,0 +1,44 @@ +# Disallow returning from an async test or hook (no-return-from-async) + +Mocha's tests or hooks (like `before`) may be asynchronous by returning a Promise. When such a Promise-returning function is defined using [an ES7 `async` function](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function) it can be confusing when combined with an explicit `return` of a Promise, as it's mixing the two styles. + +## Rule Details + +This rule looks for every test and hook (`before`, `after`, `beforeEach` and `afterEach`) and reports when the function is async and returns a value. Returning a non-Promise value is fine from Mocha's perspective, though it is ignored, but helps the linter catch more error cases. + +The following patterns are considered warnings: + +```js +describe('suite', function () { + before('title', async function() { + return foo; + }); + + it('title', async function() { + return bar().then(function() { + quux(); + }); + }); +}); +``` + +These patterns would not be considered warnings: + +```js +describe('suite', function () { + before('title', async function() { + await foo(); + }); + + it('title', function() { + if (bailEarly) { + return; + } + await bar(); + }); +}); +``` + +## When Not To Use It + +* If you use another library which exposes a similar API as mocha (e.g. `before`, `after`), you should turn this rule off, because it would raise warnings. diff --git a/index.js b/index.js index 76cb7c1..06961f3 100644 --- a/index.js +++ b/index.js @@ -9,6 +9,7 @@ module.exports = { 'no-synchronous-tests': require('./lib/rules/no-synchronous-tests'), 'no-global-tests': require('./lib/rules/no-global-tests'), 'no-return-and-callback': require('./lib/rules/no-return-and-callback'), + 'no-return-from-async': require('./lib/rules/no-return-from-async'), 'valid-test-description': require('./lib/rules/valid-test-description'), 'valid-suite-description': require('./lib/rules/valid-suite-description'), 'no-mocha-arrows': require('./lib/rules/no-mocha-arrows'), diff --git a/lib/rules/no-return-from-async.js b/lib/rules/no-return-from-async.js new file mode 100644 index 0000000..4bdba0e --- /dev/null +++ b/lib/rules/no-return-from-async.js @@ -0,0 +1,69 @@ +'use strict'; + +const R = require('ramda'); +const astUtils = require('../util/ast'); + +const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); + +function hasParentMochaFunctionCall(functionExpression) { + return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); +} + +function reportIfShortArrowFunction(context, node) { + if (node.body.type !== 'BlockStatement') { + context.report({ + node: node.body, + message: 'Confusing implicit return in a test with an async function' + }); + return true; + } + return false; +} + +function isExplicitUndefined(node) { + return node && node.type === 'Identifier' && node.name === 'undefined'; +} + +function isReturnOfUndefined(node) { + const argument = node.argument; + const isImplicitUndefined = argument === null; + + return isImplicitUndefined || isExplicitUndefined(argument); +} + +function isAllowedReturnStatement(node) { + const argument = node.argument; + + if (isReturnOfUndefined(node) || argument.type === 'Literal') { + return true; + } + + return false; +} + +function reportIfFunctionWithBlock(context, node) { + const returnStatement = findReturnStatement(node.body.body); + if (returnStatement && !isAllowedReturnStatement(returnStatement)) { + context.report({ + node: returnStatement, + message: 'Unexpected use of `return` in a test with an async function' + }); + } +} + +module.exports = function (context) { + function check(node) { + if (!node.async || !hasParentMochaFunctionCall(node)) { + return; + } + + if (!reportIfShortArrowFunction(context, node)) { + reportIfFunctionWithBlock(context, node); + } + } + + return { + FunctionExpression: check, + ArrowFunctionExpression: check + }; +}; diff --git a/test/rules/no-return-from-async.js b/test/rules/no-return-from-async.js new file mode 100644 index 0000000..859c137 --- /dev/null +++ b/test/rules/no-return-from-async.js @@ -0,0 +1,135 @@ +'use strict'; + +const RuleTester = require('eslint').RuleTester; +const rules = require('../../').rules; +const ruleTester = new RuleTester(); +const message = 'Unexpected use of `return` in a test with an async function'; +const es6parserOptions = { + sourceType: 'module', + ecmaVersion: 8 +}; + +ruleTester.run('no-return-from-async', rules['no-return-from-async'], { + + valid: [ + { + code: 'it("title", async function() {});', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { async function other() { return foo.then(function () {}); } });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { const bar = async () => { return foo.then(function () {}); }; });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { const bar = { async a() { return foo.then(function () {}); } }; });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async () => {});', + parserOptions: es6parserOptions + }, + { + code: 'it.only("title", async function() {});', + parserOptions: es6parserOptions + }, + { + code: 'before("title", async function() {});', + parserOptions: es6parserOptions + }, + { + code: 'after("title", async function() {});', + parserOptions: es6parserOptions + }, + { + code: 'async function foo() { return foo.then(function () {}); }', + parserOptions: es6parserOptions + }, + { + code: 'var foo = async function() { return foo.then(function () {}); }', + parserOptions: es6parserOptions + }, + { + code: 'notMocha("title", async function() { return foo.then(function () {}); })', + parserOptions: es6parserOptions + }, + // Allowed return statements + { + code: 'it("title", async function() { return; });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { return undefined; });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { return null; });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { return "3"; });', + parserOptions: es6parserOptions + } + ], + + invalid: [ + { + code: 'it("title", async function() { return foo; });', + errors: [ { message, column: 32, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { return foo.then(function() {}).catch(function() {}); });', + errors: [ { message, column: 32, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it("title", async function() { var foo = bar(); return foo.then(function() {}); });', + errors: [ { message, column: 49, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it("title", async () => { return foo.then(function() {}).catch(function() {}); });', + errors: [ { message, column: 27, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it("title", async () => foo.then(function() {}));', + errors: [ { message: 'Confusing implicit return in a test with an async function', column: 25, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it.only("title", async function() { return foo.then(function () {}); });', + errors: [ { message, column: 37, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'before("title", async function() { return foo.then(function() {}); });', + errors: [ { message, column: 36, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'beforeEach("title", async function() { return foo.then(function() {}); });', + errors: [ { message, column: 40, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'after("title", async function() { return foo.then(function() {}); });', + errors: [ { message, column: 35, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'afterEach("title", async function() { return foo.then(function() {}); });', + errors: [ { message, column: 39, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'afterEach("title", async function() { return foo; });', + errors: [ { message, column: 39, line: 1 } ], + parserOptions: es6parserOptions + } + ] +}); From c4928aba33ffdda761690134323ad40523ef79ab Mon Sep 17 00:00:00 2001 From: Andreas Lind Date: Tue, 16 Jul 2019 22:23:17 +0200 Subject: [PATCH 2/5] Move findReturnStatement into astUtils --- lib/rules/no-return-and-callback.js | 5 +---- lib/rules/no-return-from-async.js | 5 +---- lib/util/ast.js | 5 ++++- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/rules/no-return-and-callback.js b/lib/rules/no-return-and-callback.js index 79e5a1b..0898fc9 100644 --- a/lib/rules/no-return-and-callback.js +++ b/lib/rules/no-return-and-callback.js @@ -1,10 +1,7 @@ 'use strict'; -const R = require('ramda'); const astUtils = require('../util/ast'); -const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); - function hasParentMochaFunctionCall(functionExpression) { return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); } @@ -48,7 +45,7 @@ function isAllowedReturnStatement(node, doneName) { } function reportIfFunctionWithBlock(context, node, doneName) { - const returnStatement = findReturnStatement(node.body.body); + const returnStatement = astUtils.findReturnStatement(node.body.body); if (returnStatement && !isAllowedReturnStatement(returnStatement, doneName)) { context.report({ node: returnStatement, diff --git a/lib/rules/no-return-from-async.js b/lib/rules/no-return-from-async.js index 4bdba0e..31615e0 100644 --- a/lib/rules/no-return-from-async.js +++ b/lib/rules/no-return-from-async.js @@ -1,10 +1,7 @@ 'use strict'; -const R = require('ramda'); const astUtils = require('../util/ast'); -const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); - function hasParentMochaFunctionCall(functionExpression) { return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); } @@ -42,7 +39,7 @@ function isAllowedReturnStatement(node) { } function reportIfFunctionWithBlock(context, node) { - const returnStatement = findReturnStatement(node.body.body); + const returnStatement = astUtils.findReturnStatement(node.body.body); if (returnStatement && !isAllowedReturnStatement(returnStatement)) { context.report({ node: returnStatement, diff --git a/lib/util/ast.js b/lib/util/ast.js index 6f9a561..192c2f7 100644 --- a/lib/util/ast.js +++ b/lib/util/ast.js @@ -77,6 +77,8 @@ function isStringLiteral(node) { return node && node.type === 'Literal' && typeof node.value === 'string'; } +const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); + module.exports = { isDescribe, isHookIdentifier, @@ -85,5 +87,6 @@ module.exports = { getNodeName, isMochaFunctionCall, isHookCall, - isStringLiteral + isStringLiteral, + findReturnStatement }; From 7d7eed81805017e5e822bfaed8c6a4d4879491b4 Mon Sep 17 00:00:00 2001 From: Andreas Lind Date: Tue, 16 Jul 2019 22:24:51 +0200 Subject: [PATCH 3/5] Move hasParentMochaFunctionCall into astUtils --- lib/rules/no-return-and-callback.js | 6 +----- lib/rules/no-return-from-async.js | 6 +----- lib/util/ast.js | 5 +++++ 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/rules/no-return-and-callback.js b/lib/rules/no-return-and-callback.js index 0898fc9..7d6a26c 100644 --- a/lib/rules/no-return-and-callback.js +++ b/lib/rules/no-return-and-callback.js @@ -2,10 +2,6 @@ const astUtils = require('../util/ast'); -function hasParentMochaFunctionCall(functionExpression) { - return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); -} - function reportIfShortArrowFunction(context, node) { if (node.body.type !== 'BlockStatement') { context.report({ @@ -56,7 +52,7 @@ function reportIfFunctionWithBlock(context, node, doneName) { module.exports = function (context) { function check(node) { - if (node.params.length === 0 || !hasParentMochaFunctionCall(node)) { + if (node.params.length === 0 || !astUtils.hasParentMochaFunctionCall(node)) { return; } diff --git a/lib/rules/no-return-from-async.js b/lib/rules/no-return-from-async.js index 31615e0..750b3ad 100644 --- a/lib/rules/no-return-from-async.js +++ b/lib/rules/no-return-from-async.js @@ -2,10 +2,6 @@ const astUtils = require('../util/ast'); -function hasParentMochaFunctionCall(functionExpression) { - return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); -} - function reportIfShortArrowFunction(context, node) { if (node.body.type !== 'BlockStatement') { context.report({ @@ -50,7 +46,7 @@ function reportIfFunctionWithBlock(context, node) { module.exports = function (context) { function check(node) { - if (!node.async || !hasParentMochaFunctionCall(node)) { + if (!node.async || !astUtils.hasParentMochaFunctionCall(node)) { return; } diff --git a/lib/util/ast.js b/lib/util/ast.js index 192c2f7..e03263b 100644 --- a/lib/util/ast.js +++ b/lib/util/ast.js @@ -77,6 +77,10 @@ function isStringLiteral(node) { return node && node.type === 'Literal' && typeof node.value === 'string'; } +function hasParentMochaFunctionCall(functionExpression) { + return isTestCase(functionExpression.parent) || isHookCall(functionExpression.parent); +} + const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); module.exports = { @@ -88,5 +92,6 @@ module.exports = { isMochaFunctionCall, isHookCall, isStringLiteral, + hasParentMochaFunctionCall, findReturnStatement }; From 8085bfe713d8a3b03966af40fb79ffb5ad1697a4 Mon Sep 17 00:00:00 2001 From: Andreas Lind Date: Tue, 16 Jul 2019 22:26:49 +0200 Subject: [PATCH 4/5] Move isReturnOfUndefined into astUtils --- lib/rules/no-return-and-callback.js | 13 +------------ lib/rules/no-return-from-async.js | 13 +------------ lib/util/ast.js | 14 +++++++++++++- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/lib/rules/no-return-and-callback.js b/lib/rules/no-return-and-callback.js index 7d6a26c..bc1198a 100644 --- a/lib/rules/no-return-and-callback.js +++ b/lib/rules/no-return-and-callback.js @@ -13,17 +13,6 @@ function reportIfShortArrowFunction(context, node) { return false; } -function isExplicitUndefined(node) { - return node && node.type === 'Identifier' && node.name === 'undefined'; -} - -function isReturnOfUndefined(node) { - const argument = node.argument; - const isImplicitUndefined = argument === null; - - return isImplicitUndefined || isExplicitUndefined(argument); -} - function isFunctionCallWithName(node, name) { return node.type === 'CallExpression' && node.callee.type === 'Identifier' && @@ -33,7 +22,7 @@ function isFunctionCallWithName(node, name) { function isAllowedReturnStatement(node, doneName) { const argument = node.argument; - if (isReturnOfUndefined(node) || argument.type === 'Literal') { + if (astUtils.isReturnOfUndefined(node) || argument.type === 'Literal') { return true; } diff --git a/lib/rules/no-return-from-async.js b/lib/rules/no-return-from-async.js index 750b3ad..9fa453d 100644 --- a/lib/rules/no-return-from-async.js +++ b/lib/rules/no-return-from-async.js @@ -13,21 +13,10 @@ function reportIfShortArrowFunction(context, node) { return false; } -function isExplicitUndefined(node) { - return node && node.type === 'Identifier' && node.name === 'undefined'; -} - -function isReturnOfUndefined(node) { - const argument = node.argument; - const isImplicitUndefined = argument === null; - - return isImplicitUndefined || isExplicitUndefined(argument); -} - function isAllowedReturnStatement(node) { const argument = node.argument; - if (isReturnOfUndefined(node) || argument.type === 'Literal') { + if (astUtils.isReturnOfUndefined(node) || argument.type === 'Literal') { return true; } diff --git a/lib/util/ast.js b/lib/util/ast.js index e03263b..88d3063 100644 --- a/lib/util/ast.js +++ b/lib/util/ast.js @@ -81,6 +81,17 @@ function hasParentMochaFunctionCall(functionExpression) { return isTestCase(functionExpression.parent) || isHookCall(functionExpression.parent); } +function isExplicitUndefined(node) { + return node && node.type === 'Identifier' && node.name === 'undefined'; +} + +function isReturnOfUndefined(node) { + const argument = node.argument; + const isImplicitUndefined = argument === null; + + return isImplicitUndefined || isExplicitUndefined(argument); +} + const findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')); module.exports = { @@ -93,5 +104,6 @@ module.exports = { isHookCall, isStringLiteral, hasParentMochaFunctionCall, - findReturnStatement + findReturnStatement, + isReturnOfUndefined }; From bdc4b703a9f7f788e1512f7b3b2d5e5570ecca70 Mon Sep 17 00:00:00 2001 From: Andreas Lind Date: Tue, 16 Jul 2019 23:02:27 +0200 Subject: [PATCH 5/5] Dedupe two more occurrences of hasParentMochaFunctionCall --- lib/rules/handle-done-callback.js | 6 +----- lib/rules/no-synchronous-tests.js | 7 +------ 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/lib/rules/handle-done-callback.js b/lib/rules/handle-done-callback.js index 4eca3c5..bd7cdfc 100644 --- a/lib/rules/handle-done-callback.js +++ b/lib/rules/handle-done-callback.js @@ -4,10 +4,6 @@ const R = require('ramda'); const astUtils = require('../util/ast'); module.exports = function (context) { - function hasParentMochaFunctionCall(functionExpression) { - return astUtils.isTestCase(functionExpression.parent) || astUtils.isHookCall(functionExpression.parent); - } - function isAsyncFunction(functionExpression) { return functionExpression.params.length === 1; } @@ -40,7 +36,7 @@ module.exports = function (context) { } function check(node) { - if (hasParentMochaFunctionCall(node) && isAsyncFunction(node)) { + if (astUtils.hasParentMochaFunctionCall(node) && isAsyncFunction(node)) { checkAsyncMochaFunction(node); } } diff --git a/lib/rules/no-synchronous-tests.js b/lib/rules/no-synchronous-tests.js index 3d10efa..ec2ad49 100644 --- a/lib/rules/no-synchronous-tests.js +++ b/lib/rules/no-synchronous-tests.js @@ -5,11 +5,6 @@ const astUtil = require('../util/ast'); const asyncMethods = [ 'async', 'callback', 'promise' ]; -function hasParentMochaFunctionCall(functionExpression) { - return astUtil.isTestCase(functionExpression.parent) || - astUtil.isHookIdentifier(functionExpression.parent.callee); -} - function hasAsyncCallback(functionExpression) { return functionExpression.params.length === 1; } @@ -44,7 +39,7 @@ module.exports = function (context) { const allowedAsyncMethods = R.isNil(options.allowed) ? asyncMethods : options.allowed; function check(node) { - if (hasParentMochaFunctionCall(node)) { + if (astUtil.hasParentMochaFunctionCall(node)) { // For each allowed async test method, check if it is used in the test const testAsyncMethods = allowedAsyncMethods.map(function (method) { switch (method) {