diff --git a/docs/rules/README.md b/docs/rules/README.md index 6fa5e63..50b2133 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -6,6 +6,7 @@ * [handle-done-callback](handle-done-callback.md) - enforces handling of callbacks for async tests * [no-synchronous-tests](no-synchronous-tests.md) - disallow synchronous tests * [no-global-tests](no-global-tests.md) - disallow global tests +* [no-return-and-callback](no-return-and-callback.md) - disallow returning in a test or hook function that uses a callback * [valid-test-description](valid-test-description.md) - match test descriptions against a pre-configured regular expression * [valid-suite-description](valid-suite-description.md) - match suite descriptions against a pre-configured regular expression * [no-sibling-hooks](no-sibling-hooks.md) - disallow duplicate uses of a hook at the same level inside a describe diff --git a/docs/rules/no-return-and-callback.md b/docs/rules/no-return-and-callback.md new file mode 100644 index 0000000..1707d3c --- /dev/null +++ b/docs/rules/no-return-and-callback.md @@ -0,0 +1,41 @@ +# Disallow returning in a test or hook function that uses a callback (no-return-and-callback) + +Mocha's tests or hooks (like `before`) may be asynchronous by either returning a Promise or specifying a callback parameter for the function. It can be confusing to have both methods used in a test or hook, and from Mocha v3 on, causes the test to fail in order to force developers to remove this source of confusion. + +## Rule Details + +This rule looks for every test and hook (`before`, `after`, `beforeEach` and `afterEach`) and reports when the function takes a parameter 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', function(done) { + return foo(done); + }); + + it('title', function(done) { + return bar().then(function() { + done(); + }); + }); +}); +``` + +These patterns would not be considered warnings: + +```js +describe('suite', function () { + before('title', function(done) { + foo(done); + }); + + it('title', function() { + return 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 ef433da..22d6d61 100644 --- a/index.js +++ b/index.js @@ -8,6 +8,7 @@ module.exports = { 'handle-done-callback': require('./lib/rules/handle-done-callback'), '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'), '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-and-callback.js b/lib/rules/no-return-and-callback.js new file mode 100644 index 0000000..a3c193c --- /dev/null +++ b/lib/rules/no-return-and-callback.js @@ -0,0 +1,86 @@ +'use strict'; + +var R = require('ramda'), + findReturnStatement = R.find(R.propEq('type', 'ReturnStatement')), + possibleAsyncFunctionNames = [ + 'it', + 'it.only', + 'test', + 'test.only', + 'specify', + 'specify.only', + 'before', + 'after', + 'beforeEach', + 'afterEach' + ]; + +function getCalleeName(callee) { + if (callee.type === 'MemberExpression') { + return callee.object.name + '.' + callee.property.name; + } + + return callee.name; +} + +function hasParentMochaFunctionCall(functionExpression) { + var name; + + if (functionExpression.parent && functionExpression.parent.type === 'CallExpression') { + name = getCalleeName(functionExpression.parent.callee); + return possibleAsyncFunctionNames.indexOf(name) > -1; + } + + return false; +} + +function reportIfShortArrowFunction(context, node) { + if (node.body.type !== 'BlockStatement') { + context.report({ + node: node.body, + message: 'Confusing implicit return in a test with callback' + }); + return true; + } + return false; +} + +function isAllowedReturnStatement(node, doneName) { + var argument = node.argument; + if (argument === null || argument.type === 'Literal') { + return true; + } + if (argument.type === 'Identifier' && argument.name === 'undefined') { + return true; + } + return argument.type === 'CallExpression' && + argument.callee.type === 'Identifier' && + argument.callee.name === doneName; +} + +function reportIfFunctionWithBlock(context, node, doneName) { + var returnStatement = findReturnStatement(node.body.body); + if (returnStatement && !isAllowedReturnStatement(returnStatement, doneName)) { + context.report({ + node: returnStatement, + message: 'Unexpected use of `return` in a test with callback' + }); + } +} + +module.exports = function (context) { + function check(node) { + if (node.params.length === 0 || !hasParentMochaFunctionCall(node)) { + return; + } + + if (!reportIfShortArrowFunction(context, node)) { + reportIfFunctionWithBlock(context, node, node.params[0].name); + } + } + + return { + FunctionExpression: check, + ArrowFunctionExpression: check + }; +}; diff --git a/test/index.js b/test/index.js index c274d1c..90e086c 100644 --- a/test/index.js +++ b/test/index.js @@ -57,7 +57,7 @@ describe('eslint-plugin-mocha', function () { }); }); - it('should be linked in the documenation index', function () { + it('should be linked in the documentation index', function () { documentationFiles.forEach(function (file) { var ruleName = path.basename(file, '.md'), expectedLink = '* [' + ruleName + '](' + file + ')'; diff --git a/test/rules/no-return-and-callback.js b/test/rules/no-return-and-callback.js new file mode 100644 index 0000000..72ba7cd --- /dev/null +++ b/test/rules/no-return-and-callback.js @@ -0,0 +1,124 @@ +'use strict'; + +var RuleTester = require('eslint').RuleTester, + rules = require('../../').rules, + ruleTester = new RuleTester(), + message = 'Unexpected use of `return` in a test with callback', + es6parserOptions = { + sourceType: 'module', + ecmaVersion: 6 + }; + +ruleTester.run('no-return-and-callback', rules['no-return-and-callback'], { + + valid: [ + 'it("title", function(done) { done(); });', + 'it("title", function(done) { foo.then(function() { return done(); }); });', + 'it("title", function(done) { foo(function() { return done(); }); });', + 'it("title", function() { return foo(); });', + 'it.only("title", function(done) { done(); });', + 'it.only("title", function(done) { foo.then(function() { return done(); }); });', + 'it.only("title", function(done) { foo(function() { return done(); }); });', + 'it.only("title", function() { return foo(); });', + 'before("title", function(done) { done(); });', + 'before("title", function(done) { foo.then(function() { return done(); }); });', + 'before("title", function(done) { foo(function() { return done(); }); });', + 'before("title", function() { return foo(); });', + 'after("title", function(done) { done(); });', + 'after("title", function(done) { foo.then(function() { return done(); }); });', + 'after("title", function(done) { foo(function() { return done(); }); });', + 'after("title", function() { return foo(); });', + 'function foo(done) { return foo.then(done); }', + 'var foo = function(done) { return foo.then(done); }', + 'notMocha("title", function(done) { return foo.then(done); })', + { + code: 'it("title", (done) => { done(); });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", (done) => { foo.then(function() { return done(); }); });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", (done) => { foo(function() { return done(); }); });', + parserOptions: es6parserOptions + }, + { + code: 'it("title", () => { return foo(); });', + parserOptions: es6parserOptions + }, + // Allowed return statements + 'it("title", function(done) { return; });', + 'it("title", function(done) { return undefined; });', + 'it("title", function(done) { return null; });', + 'it("title", function(done) { return "3"; });', + 'it("title", function(done) { return done(); });', + 'it("title", function(done) { return done(error); });' + ], + + invalid: [ + { + code: 'it("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 30, line: 1 } ] + }, + { + code: 'it("title", function(done) { return foo.then(function() { done(); }).catch(done); });', + errors: [ { message: message, column: 30, line: 1 } ] + }, + { + code: 'it("title", function(done) { var foo = bar(); return foo.then(function() { done(); }); });', + errors: [ { message: message, column: 47, line: 1 } ] + }, + { + code: 'it("title", (done) => { return foo.then(function() { done(); }).catch(done); });', + errors: [ { message: message, column: 25, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it("title", (done) => foo.then(function() { done(); }));', + errors: [ { message: 'Confusing implicit return in a test with callback', column: 23, line: 1 } ], + parserOptions: es6parserOptions + }, + { + code: 'it.only("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 35, line: 1 } ] + }, + { + code: 'before("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 34, line: 1 } ] + }, + { + code: 'beforeEach("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 38, line: 1 } ] + }, + { + code: 'after("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 33, line: 1 } ] + }, + { + code: 'afterEach("title", function(done) { return foo.then(done); });', + errors: [ { message: message, column: 37, line: 1 } ] + }, + { + code: 'afterEach("title", function(done) { return foo; });', + errors: [ { message: message, column: 37, line: 1 } ] + }, + { + code: 'afterEach("title", function(done) { return done; });', + errors: [ { message: message, column: 37, line: 1 } ] + }, + { + code: 'afterEach("title", function(done) { return done.foo(); });', + errors: [ { message: message, column: 37, line: 1 } ] + }, + { + code: 'afterEach("title", function(done) { return foo.done(); });', + errors: [ { message: message, column: 37, line: 1 } ] + }, + { + code: 'afterEach("title", function(end) { return done(); });', + errors: [ { message: message, column: 36, line: 1 } ] + } + ] + +});