feat: fail tests when multiple done() calls are made (#10624)

jestjs · Dec 22, 2020 · 33b8df1 · 33b8df1
1 parent e32e274
commit 33b8df1
Show file tree

Hide file tree

Showing 13 changed files with 264 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ### Features
 
+- `[jest-circus]` [**BREAKING**] Fail tests when multiple `done()` calls are made ([#10624](https://github.com/facebook/jest/pull/10624))
 - `[jest-circus, jest-jasmine2]` [**BREAKING**] Fail the test instead of just warning when describe returns a value ([#10947](https://github.com/facebook/jest/pull/10947))
 - `[jest-config]` [**BREAKING**] Default to Node testing environment instead of browser (JSDOM) ([#9874](https://github.com/facebook/jest/pull/9874))
 - `[jest-config]` [**BREAKING**] Use `jest-circus` as default test runner ([#10686](https://github.com/facebook/jest/pull/10686))

diff --git a/e2e/__tests__/__snapshots__/callDoneTwice.test.ts.snap b/e2e/__tests__/__snapshots__/callDoneTwice.test.ts.snap
@@ -0,0 +1,89 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`\`done()\` should not be called more than once 1`] = `
+FAIL __tests__/index.test.js
+  ● \`done()\` called more than once › should fail
+
+    Expected done to be called once, but it was called multiple times.
+
+       8 |   it('should fail', done => {
+       9 |     done();
+    > 10 |     done();
+         |     ^
+      11 |   });
+      12 | 
+      13 |   it('should fail inside a promise', done => {
+
+      at Object.done (__tests__/index.test.js:10:5)
+
+  ● \`done()\` called more than once › should fail inside a promise
+
+    Expected done to be called once, but it was called multiple times.
+
+      15 |       .then(() => {
+      16 |         done();
+    > 17 |         done();
+         |         ^
+      18 |       })
+      19 |       .catch(err => err);
+      20 |   });
+
+      at done (__tests__/index.test.js:17:9)
+
+  ● multiple \`done()\` inside beforeEach › should fail
+
+    Expected done to be called once, but it was called multiple times.
+
+      24 |   beforeEach(done => {
+      25 |     done();
+    > 26 |     done();
+         |     ^
+      27 |   });
+      28 | 
+      29 |   it('should fail', () => {
+
+      at Object.done (__tests__/index.test.js:26:5)
+
+  ● multiple \`done()\` inside afterEach › should fail
+
+    Expected done to be called once, but it was called multiple times.
+
+      35 |   afterEach(done => {
+      36 |     done();
+    > 37 |     done();
+         |     ^
+      38 |   });
+      39 | 
+      40 |   it('should fail', () => {
+
+      at Object.done (__tests__/index.test.js:37:5)
+
+  ● multiple \`done()\` inside beforeAll › should fail
+
+    Expected done to be called once, but it was called multiple times.
+
+      46 |   beforeAll(done => {
+      47 |     done();
+    > 48 |     done();
+         |     ^
+      49 |   });
+      50 | 
+      51 |   it('should fail', () => {
+
+      at done (__tests__/index.test.js:48:5)
+
+
+  ● Test suite failed to run
+
+    Expected done to be called once, but it was called multiple times.
+
+      57 |   afterAll(done => {
+      58 |     done();
+    > 59 |     done();
+         |     ^
+      60 |   });
+      61 | 
+      62 |   it('should fail', () => {
+
+      at done (__tests__/index.test.js:59:5)
+`;
diff --git a/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap b/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap
@@ -16,7 +16,7 @@ FAIL __tests__/asyncDefinition.test.js
       14 |   });
       15 | });
 
-      at eventHandler (../../packages/jest-circus/build/eventHandler.js:144:11)
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:146:11)
       at test (__tests__/asyncDefinition.test.js:12:5)
 
   ● Test suite failed to run
@@ -31,7 +31,7 @@ FAIL __tests__/asyncDefinition.test.js
       15 | });
       16 | 
 
-      at eventHandler (../../packages/jest-circus/build/eventHandler.js:113:11)
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:114:11)
       at afterAll (__tests__/asyncDefinition.test.js:13:5)
 
   ● Test suite failed to run
@@ -46,7 +46,7 @@ FAIL __tests__/asyncDefinition.test.js
       20 | });
       21 | 
 
-      at eventHandler (../../packages/jest-circus/build/eventHandler.js:144:11)
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:146:11)
       at test (__tests__/asyncDefinition.test.js:18:3)
 
   ● Test suite failed to run
@@ -60,6 +60,6 @@ FAIL __tests__/asyncDefinition.test.js
       20 | });
       21 | 
 
-      at eventHandler (../../packages/jest-circus/build/eventHandler.js:113:11)
+      at eventHandler (../../packages/jest-circus/build/eventHandler.js:114:11)
       at afterAll (__tests__/asyncDefinition.test.js:19:3)
 `;
diff --git a/e2e/__tests__/callDoneTwice.test.ts b/e2e/__tests__/callDoneTwice.test.ts
@@ -0,0 +1,18 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+import wrap from 'jest-snapshot-serializer-raw';
+import {skipSuiteOnJasmine} from '@jest/test-utils';
+import {extractSummary} from '../Utils';
+import runJest from '../runJest';
+
+skipSuiteOnJasmine();
+test('`done()` should not be called more than once', () => {
+  const {exitCode, stderr} = runJest('call-done-twice');
+  const {rest} = extractSummary(stderr);
+  expect(wrap(rest)).toMatchSnapshot();
+  expect(exitCode).toBe(1);
+});
diff --git a/e2e/__tests__/doneInHooks.test.ts b/e2e/__tests__/doneInHooks.test.ts
@@ -0,0 +1,15 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+import {skipSuiteOnJasmine} from '@jest/test-utils';
+import runJest from '../runJest';
+
+skipSuiteOnJasmine();
+test('`done()` works properly in hooks', () => {
+  const {exitCode} = runJest('done-in-hooks');
+  expect(exitCode).toEqual(0);
+});
diff --git a/e2e/call-done-twice/__tests__/index.test.js b/e2e/call-done-twice/__tests__/index.test.js
@@ -0,0 +1,65 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+describe('`done()` called more than once', () => {
+  it('should fail', done => {
+    done();
+    done();
+  });
+
+  it('should fail inside a promise', done => {
+    Promise.resolve()
+      .then(() => {
+        done();
+        done();
+      })
+      .catch(err => err);
+  });
+});
+
+describe('multiple `done()` inside beforeEach', () => {
+  beforeEach(done => {
+    done();
+    done();
+  });
+
+  it('should fail', () => {
+    expect('foo').toMatch('foo');
+  });
+});
+
+describe('multiple `done()` inside afterEach', () => {
+  afterEach(done => {
+    done();
+    done();
+  });
+
+  it('should fail', () => {
+    expect('foo').toMatch('foo');
+  });
+});
+
+describe('multiple `done()` inside beforeAll', () => {
+  beforeAll(done => {
+    done();
+    done();
+  });
+
+  it('should fail', () => {
+    expect('foo').toMatch('foo');
+  });
+});
+
+describe('multiple `done()` inside afterAll', () => {
+  afterAll(done => {
+    done();
+    done();
+  });
+
+  it('should fail', () => {
+    expect('foo').toMatch('foo');
+  });
+});
diff --git a/e2e/call-done-twice/package.json b/e2e/call-done-twice/package.json
@@ -0,0 +1,5 @@
+{
+    "jest": {
+      "testEnvironment": "node"
+    }
+}
diff --git a/e2e/done-in-hooks/__tests__/index.test.js b/e2e/done-in-hooks/__tests__/index.test.js
@@ -0,0 +1,16 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ */
+describe('`done()` should work with hooks', done => {
+  beforeEach(done => done());
+  it('foo', () => {
+    expect('foo').toMatch('foo');
+  });
+  it('bar', () => {
+    expect('bar').toMatch('bar');
+  });
+});
diff --git a/e2e/done-in-hooks/package.json b/e2e/done-in-hooks/package.json
@@ -0,0 +1,5 @@
+{
+  "jest": {
+    "testEnvironment": "node"
+  }
+}
diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts
@@ -31,6 +31,7 @@ const eventHandler: Circus.EventHandler = (
       break;
     }
     case 'hook_start': {
+      event.hook.seenDone = false;
       break;
     }
     case 'start_describe_definition': {
@@ -113,7 +114,14 @@ const eventHandler: Circus.EventHandler = (
       }
       const parent = currentDescribeBlock;
 
-      currentDescribeBlock.hooks.push({asyncError, fn, parent, timeout, type});
+      currentDescribeBlock.hooks.push({
+        asyncError,
+        fn,
+        parent,
+        seenDone: false,
+        timeout,
+        type,
+      });
       break;
     }
     case 'add_test': {
@@ -188,6 +196,10 @@ const eventHandler: Circus.EventHandler = (
       event.test.invocations += 1;
       break;
     }
+    case 'test_fn_start': {
+      event.test.seenDone = false;
+      break;
+    }
     case 'test_fn_failure': {
       const {
         error,

diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts
@@ -70,6 +70,7 @@ export const makeTest = (
   mode,
   name: convertDescriptorToString(name),
   parent,
+  seenDone: false,
   startedAt: null,
   status: null,
   timeout,
@@ -189,9 +190,25 @@ export const callAsyncCircusFn = (
     // soon as `done` called.
     if (takesDoneCallback(fn)) {
       let returnedValue: unknown = undefined;
+
       const done = (reason?: Error | string): void => {
         // We need to keep a stack here before the promise tick
         const errorAtDone = new ErrorWithStack(undefined, done);
+
+        if (!completed && testOrHook.seenDone) {
+          errorAtDone.message =
+            'Expected done to be called once, but it was called multiple times.';
+
+          if (reason) {
+            errorAtDone.message +=
+              ' Reason: ' + prettyFormat(reason, {maxDepth: 3});
+          }
+          reject(errorAtDone);
+          throw errorAtDone;
+        } else {
+          testOrHook.seenDone = true;
+        }
+
         // Use `Promise.resolve` to allow the event loop to go a single tick in case `done` is called synchronously
         Promise.resolve().then(() => {
           if (returnedValue !== undefined) {
@@ -203,7 +220,6 @@ export const callAsyncCircusFn = (
           }
 
           let errorAsErrorObject: Error;
-
           if (checkIsError(reason)) {
             errorAsErrorObject = reason;
           } else {
@@ -274,12 +290,12 @@ export const callAsyncCircusFn = (
       completed = true;
       // If timeout is not cleared/unrefed the node process won't exit until
       // it's resolved.
-      timeoutID.unref && timeoutID.unref();
+      timeoutID.unref?.();
       clearTimeout(timeoutID);
     })
     .catch(error => {
       completed = true;
-      timeoutID.unref && timeoutID.unref();
+      timeoutID.unref?.();
       clearTimeout(timeoutID);
       throw error;
     });