From 7344607ec30da3ee56acb68ca4018bcf4718ad74 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Tue, 23 Oct 2018 00:53:12 +0200 Subject: [PATCH] feat(rules): add `no-test-callback` rule (#179) --- README.md | 2 + docs/rules/no-test-callback.md | 76 ++++++++++++++ index.js | 2 + rules/__tests__/no-test-callback.test.js | 127 +++++++++++++++++++++++ rules/no-test-callback.js | 80 ++++++++++++++ 5 files changed, 287 insertions(+) create mode 100644 docs/rules/no-test-callback.md create mode 100644 rules/__tests__/no-test-callback.test.js create mode 100644 rules/no-test-callback.js diff --git a/README.md b/README.md index e7388e049..96d62e4b8 100644 --- a/README.md +++ b/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 diff --git a/docs/rules/no-test-callback.md b/docs/rules/no-test-callback.md new file mode 100644 index 000000000..304fbf2d0 --- /dev/null +++ b/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(); + }); +}); +``` diff --git a/index.js b/index.js index 56aa28705..42d10b311 100644 --- a/index.js +++ b/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, }, }; diff --git a/rules/__tests__/no-test-callback.test.js b/rules/__tests__/no-test-callback.test.js new file mode 100644 index 000000000..4c37bcb85 --- /dev/null +++ b/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();})})', + }, + ], +}); diff --git a/rules/no-test-callback.js b/rules/no-test-callback.js new file mode 100644 index 000000000..b20c497f0 --- /dev/null +++ b/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), + ]; + }, + }); + }, + }; + }, +};