feat(rules): add no-test-callback rule (#179)

Author: SimenB

README.md

@@ -91,6 +91,7 @@ for more information about extending configuration files.
 | [no-jasmine-globals][]       | Disallow Jasmine globals                                          |                  | ![fixable-yellow][] |
 | [no-jest-import][]           | Disallow importing `jest`                                         | ![recommended][] |                     |
 | [no-large-snapshots][]       | Disallow large snapshots                                          |                  |                     |
+| [no-test-callback][]         | Using a callback in asynchronous tests                            |                  | ![fixable-green][]  |
 | [no-test-prefixes][]         | Disallow using `f` & `x` prefixes to define focused/skipped tests |                  | ![fixable-green][]  |
 | [no-test-return-statement][] | Disallow explicitly returning from tests                          |                  |                     |
 | [prefer-expect-assertions][] | Suggest using `expect.assertions()` OR `expect.hasAssertions()`   |                  |                     |
@@ -121,6 +122,7 @@ for more information about extending configuration files.
 [no-jasmine-globals]: docs/rules/no-jasmine-globals.md
 [no-jest-import]: docs/rules/no-jest-import.md
 [no-large-snapshots]: docs/rules/no-large-snapshots.md
+[no-test-callback]: docs/rules/no-test-callback.md
 [no-test-prefixes]: docs/rules/no-test-prefixes.md
 [no-test-return-statement]: docs/rules/no-test-return-statement.md
 [prefer-expect-assertions]: docs/rules/prefer-expect-assertions.md

docs/rules/no-test-callback.md

@@ -0,0 +1,76 @@
+# Avoid using a callback in asynchronous tests (no-test-callback)
+
+Jest allows you to pass a callback to test definitions, typically called `done`,
+that is later invoked to indicate that the asynchronous test is complete.
+
+However, that means that if your test throws (e.g. because of a failing
+assertion), `done` will never be called unless you manually use `try-catch`.
+
+```js
+test('some test', done => {
+  expect(false).toBe(true);
+  done();
+});
+```
+
+The test above will time out instead of failing the assertions, since `done` is
+never called.
+
+Correct way of doing the same thing is to wrap it in `try-catch`.
+
+```js
+test('some test', done => {
+  try {
+    expect(false).toBe(true);
+    done();
+  } catch (e) {
+    done(e);
+  }
+});
+```
+
+However, Jest supports a second way of having asynchronous tests - using
+promises.
+
+```js
+test('some test', () => {
+  return new Promise(done => {
+    expect(false).toBe(true);
+    done();
+  });
+});
+```
+
+Even though `done` is never called here, the Promise will still reject, and Jest
+will report the assertion error correctly.
+
+## Rule details
+
+This rule triggers a warning if you have a `done` callback in your test.
+
+The following patterns are considered warnings:
+
+```js
+test('myFunction()', done => {
+  // ...
+});
+
+test('myFunction()', function(done) {
+  // ...
+});
+```
+
+The following patterns are not considered warnings:
+
+```js
+test('myFunction()', () => {
+  expect(myFunction()).toBeTruthy();
+});
+
+test('myFunction()', () => {
+  return new Promise(done => {
+    expect(myFunction()).toBeTruthy();
+    done();
+  });
+});
+```

index.js

@@ -24,6 +24,7 @@ const preferInlineSnapshots = require('./rules/prefer-inline-snapshots');
 const preferStrictEqual = require('./rules/prefer-strict-equal');
 const requireTothrowMessage = require('./rules/require-tothrow-message');
 const noAliasMethods = require('./rules/no-alias-methods');
+const noTestCallback = require('./rules/no-test-callback');
 
 const snapshotProcessor = require('./processors/snapshot-processor');
 
@@ -95,5 +96,6 @@ module.exports = {
     'prefer-strict-equal': preferStrictEqual,
     'require-tothrow-message': requireTothrowMessage,
     'no-alias-methods': noAliasMethods,
+    'no-test-callback': noTestCallback,
   },
 };

rules/__tests__/no-test-callback.test.js

@@ -0,0 +1,127 @@
+'use strict';
+
+const RuleTester = require('eslint').RuleTester;
+const rule = require('../no-test-callback');
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 8,
+  },
+});
+
+ruleTester.run('no-test-callback', rule, {
+  valid: [
+    'test("something", () => {})',
+    'test("something", async () => {})',
+    'test("something", function() {})',
+    'test("something", async function () {})',
+    'test("something", someArg)',
+  ],
+  invalid: [
+    {
+      code: 'test("something", done => {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 19,
+        },
+      ],
+      output:
+        'test("something", () => {return new Promise(done => {done();})})',
+    },
+    {
+      code: 'test("something", (done) => {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 20,
+        },
+      ],
+      output:
+        'test("something", () => {return new Promise((done) => {done();})})',
+    },
+    {
+      code: 'test("something", done => done())',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 19,
+        },
+      ],
+      output: 'test("something", () => new Promise(done => done()))',
+    },
+    {
+      code: 'test("something", (done) => done())',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 20,
+        },
+      ],
+      output: 'test("something", () => new Promise((done) => done()))',
+    },
+    {
+      code: 'test("something", function(done) {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 28,
+        },
+      ],
+      output:
+        'test("something", function() {return new Promise((done) => {done();})})',
+    },
+    {
+      code: 'test("something", function (done) {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 29,
+        },
+      ],
+      output:
+        'test("something", function () {return new Promise((done) => {done();})})',
+    },
+    {
+      code: 'test("something", async done => {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 25,
+        },
+      ],
+      output:
+        'test("something", async () => {await new Promise(done => {done();})})',
+    },
+    {
+      code: 'test("something", async done => done())',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 25,
+        },
+      ],
+      output: 'test("something", async () => new Promise(done => done()))',
+    },
+    {
+      code: 'test("something", async function (done) {done();})',
+      errors: [
+        {
+          message: 'Illegal usage of test callback',
+          line: 1,
+          column: 35,
+        },
+      ],
+      output:
+        'test("something", async function () {await new Promise((done) => {done();})})',
+    },
+  ],
+});

rules/no-test-callback.js

@@ -0,0 +1,80 @@
+'use strict';
+
+const getDocsUrl = require('./util').getDocsUrl;
+const isTestCase = require('./util').isTestCase;
+
+module.exports = {
+  meta: {
+    docs: {
+      url: getDocsUrl(__filename),
+    },
+    fixable: 'code',
+  },
+  create(context) {
+    return {
+      CallExpression(node) {
+        if (!isTestCase(node) || node.arguments.length !== 2) {
+          return;
+        }
+
+        const callback = node.arguments[1];
+
+        if (
+          !/^(Arrow)?FunctionExpression$/.test(callback.type) ||
+          callback.params.length !== 1
+        ) {
+          return;
+        }
+
+        const argument = callback.params[0];
+        context.report({
+          node: argument,
+          message: 'Illegal usage of test callback',
+          fix(fixer) {
+            const sourceCode = context.getSourceCode();
+            const body = callback.body;
+            const firstBodyToken = sourceCode.getFirstToken(body);
+            const lastBodyToken = sourceCode.getLastToken(body);
+            const tokenBeforeArgument = sourceCode.getTokenBefore(argument);
+            const tokenAfterArgument = sourceCode.getTokenAfter(argument);
+            const argumentInParens =
+              tokenBeforeArgument.value === '(' &&
+              tokenAfterArgument.value === ')';
+
+            let argumentFix = fixer.replaceText(argument, '()');
+
+            if (argumentInParens) {
+              argumentFix = fixer.remove(argument);
+            }
+
+            let newCallback = argument.name;
+
+            if (argumentInParens) {
+              newCallback = `(${newCallback})`;
+            }
+
+            let beforeReplacement = `new Promise(${newCallback} => `;
+            let afterReplacement = ')';
+            let replaceBefore = true;
+
+            if (body.type === 'BlockStatement') {
+              const keyword = callback.async ? 'await' : 'return';
+
+              beforeReplacement = `${keyword} ${beforeReplacement}{`;
+              afterReplacement += '}';
+              replaceBefore = false;
+            }
+
+            return [
+              argumentFix,
+              replaceBefore
+                ? fixer.insertTextBefore(firstBodyToken, beforeReplacement)
+                : fixer.insertTextAfter(firstBodyToken, beforeReplacement),
+              fixer.insertTextAfter(lastBodyToken, afterReplacement),
+            ];
+          },
+        });
+      },
+    };
+  },
+};