Implement no-return-from-async rule

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)

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.

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'),

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);
         }
     }

lib/rules/no-return-and-callback.js

@@ -1,14 +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);
-}
-
 function reportIfShortArrowFunction(context, node) {
     if (node.body.type !== 'BlockStatement') {
         context.report({
@@ -20,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' &&
@@ -40,15 +22,15 @@ 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;
     }
 
     return isFunctionCallWithName(argument, 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,
@@ -59,7 +41,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;
         }
 

lib/rules/no-return-from-async.js

@@ -0,0 +1,51 @@
+'use strict';
+
+const astUtils = require('../util/ast');
+
+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 isAllowedReturnStatement(node) {
+    const argument = node.argument;
+
+    if (astUtils.isReturnOfUndefined(node) || argument.type === 'Literal') {
+        return true;
+    }
+
+    return false;
+}
+
+function reportIfFunctionWithBlock(context, node) {
+    const returnStatement = astUtils.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 || !astUtils.hasParentMochaFunctionCall(node)) {
+            return;
+        }
+
+        if (!reportIfShortArrowFunction(context, node)) {
+            reportIfFunctionWithBlock(context, node);
+        }
+    }
+
+    return {
+        FunctionExpression: check,
+        ArrowFunctionExpression: check
+    };
+};

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) {

lib/util/ast.js

@@ -77,6 +77,23 @@ function isStringLiteral(node) {
     return node && node.type === 'Literal' && typeof node.value === 'string';
 }
 
+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 = {
     isDescribe,
     isHookIdentifier,
@@ -85,5 +102,8 @@ module.exports = {
     getNodeName,
     isMochaFunctionCall,
     isHookCall,
-    isStringLiteral
+    isStringLiteral,
+    hasParentMochaFunctionCall,
+    findReturnStatement,
+    isReturnOfUndefined
 };

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
+        }
+    ]
+});