Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for async functions in no-synchronous-tests #138

Merged
merged 2 commits into from
Jun 19, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions docs/rules/no-synchronous-tests.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,53 @@
# Disallow Synchronous Tests (no-synchronous-tests)

Mocha automatically determines whether a test is synchronous or asynchronous based on the arity of the function passed into it. When writing tests for a asynchronous function, omitting the `done` callback or forgetting to return a promise can often lead to false-positive test cases. This rule warns against the implicit synchronous feature, and should be combined with `handle-done-callback` for best results.
Mocha automatically determines whether a test is synchronous or asynchronous based on the arity of the function passed into it. When writing tests for an asynchronous function, omitting the `done` callback or forgetting to return a promise can often lead to false-positive test cases. This rule warns against the implicit synchronous feature, and should be combined with `handle-done-callback` for best results.

## Rule Details

This rule looks for either an asynchronous callback or a return statement within the function body of any mocha test statement. If the mocha callback is not used and a promise is not returned, this rule will raise a warning.
By default, this rule looks for the presence of one of:

- An asynchronous callback.
- An async function provided to a mocha test statement.
- A return statement within the function body of any mocha test statement.

If none of these three options is used in a test method, the rule will raise a warning.

The following patterns are considered warnings:

```js
it('should do foo', function () {
return;
});

it('should do foo', function () {
callback();
});
```

These patterns would not be considered warnings:

```js
it('should do foo', function (done) {
done();
});

it('should do foo', async function () {
await something();
});

it('should do foo', function () {
return promise;
}
```

You can change the acceptable asynchronous test methods to only allow a combination of async functions/callbacks/promises:

```js
rules: {
"mocha/no-synchronous-tests": ["warning", {allowed: ['async', 'callback', 'promise']}]
},
```


### Caveats:

Expand All @@ -21,7 +64,7 @@ it('test name', myTestFn);

## When Not To Use It

* If you are primarily writing synchronous tests, and rarely need the `done` callback or promise functionality.
* If you are primarily writing synchronous tests, and rarely need the `done` callback, promise functionality or async functions.

## Further Reading

Expand Down
124 changes: 87 additions & 37 deletions lib/rules/no-synchronous-tests.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
'use strict';

var R = require('ramda');

module.exports = function (context) {
var possibleAsyncFunctionNames = [
var R = require('ramda'),
defaultAllowedAsyncMethods = [ 'async', 'callback', 'promise' ],
possibleAsyncFunctionNames = [
'it',
'it.only',
'test',
Expand All @@ -16,54 +15,105 @@ module.exports = function (context) {
'afterEach'
];

function getCalleeName(callee) {
if (callee.type === 'MemberExpression') {
return callee.object.name + '.' + callee.property.name;
}

return callee.name;
function getCalleeName(callee) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this function was already there before but it would be nice to re-use the getNodeName method from our AST utils.

💅

if (callee.type === 'MemberExpression') {
return callee.object.name + '.' + callee.property.name;
}

function hasParentMochaFunctionCall(functionExpression) {
var name;
return callee.name;
}

if (functionExpression.parent && functionExpression.parent.type === 'CallExpression') {
name = getCalleeName(functionExpression.parent.callee);
return possibleAsyncFunctionNames.indexOf(name) > -1;
}
function hasParentMochaFunctionCall(functionExpression) {
var name;

return false;
if (functionExpression.parent && functionExpression.parent.type === 'CallExpression') {
name = getCalleeName(functionExpression.parent.callee);
return possibleAsyncFunctionNames.indexOf(name) > -1;
}

function hasAsyncCallback(functionExpression) {
return functionExpression.params.length === 1;
}
return false;
}

function findPromiseReturnStatement(nodes) {
return R.find(function (node) {
function hasAsyncCallback(functionExpression) {
return functionExpression.params.length === 1;
}

function isAsyncFunction(functionExpression) {
return functionExpression.async === true;
}

function findPromiseReturnStatement(nodes) {
return R.find(function (node) {
return node.type === 'ReturnStatement' && node.argument && node.argument.type !== 'Literal';
}, nodes);
}, nodes);
}

function doesReturnPromise(functionExpression) {
var bodyStatement = functionExpression.body,
returnStatement = null;

if (bodyStatement.type === 'BlockStatement') {
returnStatement = findPromiseReturnStatement(functionExpression.body.body);
} else if (bodyStatement.type === 'CallExpression') {
// allow arrow statements calling a promise with implicit return.
returnStatement = bodyStatement;
}

function checkPromiseReturn(functionExpression) {
var bodyStatement = functionExpression.body,
returnStatement = null;
return returnStatement !== null
&& typeof returnStatement !== 'undefined';
}

if (bodyStatement.type === 'BlockStatement') {
returnStatement = findPromiseReturnStatement(functionExpression.body.body);
} else if (bodyStatement.type === 'CallExpression') {
// allow arrow statements calling a promise with implicit return.
returnStatement = bodyStatement;
}
function getAllowedAsyncMethocsFromOptions(options) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo? I guess it should be getAllowedAsyncMethodsFromOptions.

if (R.isNil(options.allowed)) {
return defaultAllowedAsyncMethods;
}

if (!returnStatement) {
context.report(functionExpression, 'Unexpected synchronous test.');
}
/* istanbul ignore if */
if (!Array.isArray(options.allowed) || options.allowed.length === 0) {
throw new Error('The `allowed` option for no-synchronous-tests must be a non-empty array.');
}

return options.allowed;
}

module.exports = function (context) {
var options = context.options[0] || {},
allowedAsyncMethods = getAllowedAsyncMethocsFromOptions(options);

function check(node) {
if (hasParentMochaFunctionCall(node) && !hasAsyncCallback(node)) {
checkPromiseReturn(node);
var testAsyncMethods,
isAsyncTest;

if (hasParentMochaFunctionCall(node)) {
// For each allowed async test method, check if it is used in the test
testAsyncMethods = allowedAsyncMethods.map(function (method) {
switch (method) {
case 'async':
return isAsyncFunction(node);

case 'callback':
return hasAsyncCallback(node);

case 'promise':
return doesReturnPromise(node);

/* istanbul ignore next */
default:
throw new Error(
'Unknown async test method "' + method + '".' +
'Possible values: ' + defaultAllowedAsyncMethods.join(', ')
);
}
});

// Check that at least one allowed async test method is used in the test
isAsyncTest = testAsyncMethods.some(function (value) {
return value === true;
});

if (!isAsyncTest) {
context.report(node, 'Unexpected synchronous test.');
}
}
}

Expand Down
42 changes: 41 additions & 1 deletion test/rules/no-synchronous-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,42 @@ ruleTester.run('no-synchronous-tests', rules['no-synchronous-tests'], {
{
code: 'it("", () => promise.then() );',
parserOptions: { ecmaVersion: 6 }
},
{
code: 'it("", async function () { });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'it("", async function () { return true; });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'it("", async function (val) { return await new Promise((resolve) => { resolve(val); }); });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'before("", async function () { });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'beforeEach("", async function () { });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'after("", async function () { });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'afterEach("", async function () { });',
parserOptions: { ecmaVersion: 8 }
},
{
code: 'it("", function (done) { done(); });',
options: []
},
{
code: 'it("", function (done) { done(); });',
options: [ { } ]
}
],

Expand Down Expand Up @@ -76,7 +112,11 @@ ruleTester.run('no-synchronous-tests', rules['no-synchronous-tests'], {
{
code: 'specify.only("", function () {});',
errors: [ { message: 'Unexpected synchronous test.', column: 18, line: 1 } ]
},
{
options: [ { allowed: [ 'callback', 'async' ] } ],
code: 'it("", function () { return promise(); });',
errors: [ { message: 'Unexpected synchronous test.', column: 8, line: 1 } ]
}

]
});