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
jest-community · Sep 4, 2020 · 3e6cb44 · 3e6cb44
1 parent 99774cd
commit 3e6cb44
Show file tree

Hide file tree

Showing 4 changed files with 268 additions and 51 deletions.
diff --git 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
@@ -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
@@ -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();
+                })});
+              `,
+            },
+          ],
+        },
+      ],
+    },
   ],
 });