From 3e6cb442a20b9aea710d30f81bf2eb192d193823 Mon Sep 17 00:00:00 2001 From: Gareth Jones Date: Fri, 4 Sep 2020 22:56:17 +1200 Subject: [PATCH] feat(no-done-callback): support hooks (#656) BREAKING CHANGE: `no-done-callback` will now report hooks using callbacks as well, not just tests Fixes #649 Resolves #651 --- README.md | 2 +- docs/rules/no-done-callback.md | 72 ++++--- src/rules/__tests__/no-done-callback.test.ts | 203 ++++++++++++++++++- src/rules/no-done-callback.ts | 42 ++-- 4 files changed, 268 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 175d5d4c3..80f3bd0d6 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ installations requiring long-term consistency. | [no-conditional-expect](docs/rules/no-conditional-expect.md) | Prevent calling `expect` conditionally | ![recommended][] | | | [no-deprecated-functions](docs/rules/no-deprecated-functions.md) | Disallow use of deprecated functions | ![recommended][] | ![fixable][] | | [no-disabled-tests](docs/rules/no-disabled-tests.md) | Disallow disabled tests | ![recommended][] | | -| [no-done-callback](docs/rules/no-done-callback.md) | Avoid using a callback in asynchronous tests | ![recommended][] | ![suggest][] | +| [no-done-callback](docs/rules/no-done-callback.md) | Avoid using a callback in asynchronous tests and hooks | ![recommended][] | ![suggest][] | | [no-duplicate-hooks](docs/rules/no-duplicate-hooks.md) | Disallow duplicate setup and teardown hooks | | | | [no-export](docs/rules/no-export.md) | Disallow using `exports` in files containing tests | ![recommended][] | | | [no-focused-tests](docs/rules/no-focused-tests.md) | Disallow focused tests | ![recommended][] | ![fixable][] | diff --git a/docs/rules/no-done-callback.md b/docs/rules/no-done-callback.md index 0421296f6..b8988f023 100644 --- a/docs/rules/no-done-callback.md +++ b/docs/rules/no-done-callback.md @@ -1,56 +1,66 @@ -# Avoid using a callback in asynchronous tests (`no-done-callback`) +# Avoid using a callback in asynchronous tests and hooks (`no-done-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. +When calling asynchronous code in hooks and tests, `jest` needs to know when the +asynchronous work is complete to progress the current run. -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`. +Originally the most common pattern to archive this was to use callbacks: ```js -test('some test', done => { - expect(false).toBe(true); - done(); +test('the data is peanut butter', done => { + function callback(data) { + try { + expect(data).toBe('peanut butter'); + done(); + } catch (error) { + done(error); + } + } + + fetchData(callback); }); ``` -The test above will time out instead of failing the assertions, since `done` is -never called. +This can be very error prone however, as it requires careful understanding of +how assertions work in tests or otherwise tests won't behave as expected. -Correct way of doing the same thing is to wrap it in `try-catch`. +For example, if the `try/catch` was left out of the above code, the test would +timeout rather than fail. Even with the `try/catch`, forgetting to pass the +caught error to `done` will result in `jest` believing the test has passed. + +A more straightforward way to handle asynchronous code is to use Promises: ```js -test('some test', done => { - try { - expect(false).toBe(true); - done(); - } catch (e) { - done(e); - } +test('the data is peanut butter', () => { + return fetchData().then(data => { + expect(data).toBe('peanut butter'); + }); }); ``` -However, Jest supports a second way of having asynchronous tests - using -promises. +When a test or hook returns a promise, `jest` waits for that promise to resolve, +as well as automatically failing should the promise reject. + +If your environment supports `async/await`, this becomes even simpler: ```js -test('some test', () => { - return new Promise(done => { - expect(false).toBe(true); - done(); - }); +test('the data is peanut butter', async () => { + const data = await fetchData(); + expect(data).toBe('peanut butter'); }); ``` -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. +This rule checks the function parameter of hooks & tests for use of the `done` +argument, suggesting you return a promise instead. The following patterns are considered warnings: ```js +beforeEach(done => { + // ... +}); + test('myFunction()', done => { // ... }); @@ -63,6 +73,10 @@ test('myFunction()', function (done) { The following patterns are not considered warnings: ```js +beforeEach(async () => { + await setupUsTheBomb(); +}); + test('myFunction()', () => { expect(myFunction()).toBeTruthy(); }); diff --git a/src/rules/__tests__/no-done-callback.test.ts b/src/rules/__tests__/no-done-callback.test.ts index f53352eb8..f4fc63903 100644 --- a/src/rules/__tests__/no-done-callback.test.ts +++ b/src/rules/__tests__/no-done-callback.test.ts @@ -17,13 +17,18 @@ ruleTester.run('no-done-callback', rule, { 'test("something", function() {})', 'test("something", async function () {})', 'test("something", someArg)', + 'beforeEach(() => {})', + 'beforeAll(async () => {})', + 'afterAll(() => {})', + 'afterAll(async function () {})', + 'afterAll(async function () {}, 5)', ], invalid: [ { code: 'test("something", (...args) => {args[0]();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 20, }, @@ -33,7 +38,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", done => {done();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 19, suggestions: [ @@ -51,7 +56,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", finished => {finished();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 19, suggestions: [ @@ -69,7 +74,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", (done) => {done();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 20, suggestions: [ @@ -87,7 +92,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", done => done())', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 19, suggestions: [ @@ -104,7 +109,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", (done) => done())', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 20, suggestions: [ @@ -121,7 +126,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", function(done) {done();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 28, suggestions: [ @@ -139,7 +144,7 @@ ruleTester.run('no-done-callback', rule, { code: 'test("something", function (done) {done();})', errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 29, suggestions: [ @@ -183,7 +188,7 @@ ruleTester.run('no-done-callback', rule, { `, errors: [ { - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', line: 1, column: 20, suggestions: [ @@ -200,5 +205,185 @@ ruleTester.run('no-done-callback', rule, { }, ], }, + { + code: 'afterEach((...args) => {args[0]();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 12, + }, + ], + }, + { + code: 'beforeAll(done => {done();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 11, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: + 'beforeAll(() => {return new Promise(done => {done();})})', + }, + ], + }, + ], + }, + { + code: 'beforeAll(finished => {finished();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 11, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'finished' }, + output: + 'beforeAll(() => {return new Promise(finished => {finished();})})', + }, + ], + }, + ], + }, + { + code: 'beforeEach((done) => {done();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 13, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: + 'beforeEach(() => {return new Promise((done) => {done();})})', + }, + ], + }, + ], + }, + { + code: 'afterAll(done => done())', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 10, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: 'afterAll(() => new Promise(done => done()))', + }, + ], + }, + ], + }, + { + code: 'afterEach((done) => done())', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 12, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: 'afterEach(() => new Promise((done) => done()))', + }, + ], + }, + ], + }, + { + code: 'beforeAll(function(done) {done();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 20, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: + 'beforeAll(function() {return new Promise((done) => {done();})})', + }, + ], + }, + ], + }, + { + code: 'afterEach(function (done) {done();})', + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 21, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: + 'afterEach(function () {return new Promise((done) => {done();})})', + }, + ], + }, + ], + }, + { + code: 'beforeAll(async done => {done();})', + errors: [{ messageId: 'useAwaitInsteadOfCallback', line: 1, column: 17 }], + }, + { + code: 'beforeAll(async done => done())', + errors: [{ messageId: 'useAwaitInsteadOfCallback', line: 1, column: 17 }], + }, + { + code: 'beforeAll(async function (done) {done();})', + errors: [{ messageId: 'useAwaitInsteadOfCallback', line: 1, column: 27 }], + }, + { + code: dedent` + afterAll(async (done) => { + await myAsyncTask(); + done(); + }); + `, + errors: [{ messageId: 'useAwaitInsteadOfCallback', line: 1, column: 17 }], + }, + { + code: dedent` + beforeEach((done) => { + done(); + }); + `, + errors: [ + { + messageId: 'noDoneCallback', + line: 1, + column: 13, + suggestions: [ + { + messageId: 'suggestWrappingInPromise', + data: { callback: 'done' }, + output: dedent` + beforeEach(() => {return new Promise((done) => { + done(); + })}); + `, + }, + ], + }, + ], + }, ], }); diff --git a/src/rules/no-done-callback.ts b/src/rules/no-done-callback.ts index 2707d1fcb..70fde6caa 100644 --- a/src/rules/no-done-callback.ts +++ b/src/rules/no-done-callback.ts @@ -1,17 +1,35 @@ -import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils'; -import { createRule, isFunction, isTestCase } from './utils'; +import { + AST_NODE_TYPES, + TSESTree, +} from '@typescript-eslint/experimental-utils'; +import { createRule, isFunction, isHook, isTestCase } from './utils'; + +const findCallbackArg = ( + node: TSESTree.CallExpression, +): TSESTree.CallExpression['arguments'][0] | null => { + if (isHook(node) && node.arguments.length >= 1) { + return node.arguments[0]; + } + + if (isTestCase(node) && node.arguments.length >= 2) { + return node.arguments[1]; + } + + return null; +}; export default createRule({ name: __filename, meta: { docs: { category: 'Best Practices', - description: 'Avoid using a callback in asynchronous tests', + description: 'Avoid using a callback in asynchronous tests and hooks', recommended: 'error', suggestion: true, }, messages: { - illegalTestCallback: 'Illegal usage of test callback', + noDoneCallback: + 'Return a Promise instead of relying on callback parameter', suggestWrappingInPromise: 'Wrap in `new Promise({{ callback }} => ...`', useAwaitInsteadOfCallback: 'Use await instead of callback in async functions', @@ -23,13 +41,13 @@ export default createRule({ create(context) { return { CallExpression(node) { - if (!isTestCase(node) || node.arguments.length !== 2) { - return; - } - - const [, callback] = node.arguments; + const callback = findCallbackArg(node); - if (!isFunction(callback) || callback.params.length !== 1) { + if ( + !callback || + !isFunction(callback) || + callback.params.length !== 1 + ) { return; } @@ -38,7 +56,7 @@ export default createRule({ if (argument.type !== AST_NODE_TYPES.Identifier) { context.report({ node: argument, - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', }); return; @@ -55,7 +73,7 @@ export default createRule({ context.report({ node: argument, - messageId: 'illegalTestCallback', + messageId: 'noDoneCallback', suggest: [ { messageId: 'suggestWrappingInPromise',