Merge pull request #94 from jfmengels/issue-88

Add rule `no-return-and-callback` (fixes #88)

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

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.

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

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

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 + ')';

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