Skip to content

Commit

Permalink
Merge pull request #190 from papandreou/feature/no-return-from-async
Browse files Browse the repository at this point in the history
Implement no-return-from-async rule
  • Loading branch information
lo1tuma committed Jul 17, 2019
2 parents ae913c5 + bdc4b70 commit 31819a2
Show file tree
Hide file tree
Showing 9 changed files with 258 additions and 33 deletions.
1 change: 1 addition & 0 deletions docs/rules/README.md
Expand Up @@ -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)
Expand Down
44 changes: 44 additions & 0 deletions 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.
1 change: 1 addition & 0 deletions index.js
Expand Up @@ -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'),
Expand Down
6 changes: 1 addition & 5 deletions lib/rules/handle-done-callback.js
Expand Up @@ -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;
}
Expand Down Expand Up @@ -40,7 +36,7 @@ module.exports = function (context) {
}

function check(node) {
if (hasParentMochaFunctionCall(node) && isAsyncFunction(node)) {
if (astUtils.hasParentMochaFunctionCall(node) && isAsyncFunction(node)) {
checkAsyncMochaFunction(node);
}
}
Expand Down
24 changes: 3 additions & 21 deletions 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({
Expand All @@ -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' &&
Expand All @@ -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,
Expand All @@ -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;
}

Expand Down
51 changes: 51 additions & 0 deletions 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
};
};
7 changes: 1 addition & 6 deletions lib/rules/no-synchronous-tests.js
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down
22 changes: 21 additions & 1 deletion lib/util/ast.js
Expand Up @@ -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,
Expand All @@ -85,5 +102,8 @@ module.exports = {
getNodeName,
isMochaFunctionCall,
isHookCall,
isStringLiteral
isStringLiteral,
hasParentMochaFunctionCall,
findReturnStatement,
isReturnOfUndefined
};
135 changes: 135 additions & 0 deletions 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
}
]
});

0 comments on commit 31819a2

Please sign in to comment.