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

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-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))
 - `[jest-config, jest-runtime]` Support ESM for files other than `.js` and `.mjs` ([#10823](https://github.com/facebook/jest/pull/10823))

e2e/__tests__/__snapshots__/callDoneTwice.test.ts.snap

@@ -0,0 +1,19 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`\`done()\` should not be called more than once 1`] = `
+FAIL __tests__/index.test.js
+  ✕ \`done()\` should not be called more than once
+
+  ● \`done()\` should not be called more than once
+
+    Expected done to be called once, but it was called multiple times.
+
+       7 | test('\`done()\` should not be called more than once', done => {
+       8 |   done();
+    >  9 |   done();
+         |   ^
+      10 | });
+      11 | 
+
+      at Object.done (__tests__/index.test.js:9:3)
+`;

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:145:11)
       at test (__tests__/asyncDefinition.test.js:12: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:145:11)
       at test (__tests__/asyncDefinition.test.js:18:3)
 
   ● Test suite failed to run

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);
+});

e2e/__tests__/doneInHooks.test.ts

@@ -0,0 +1,13 @@
+/**
+ * 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 runJest from '../runJest';
+
+test('`done()` works properly in hooks', () => {
+  const {exitCode} = runJest('done-in-hooks');
+  expect(exitCode).toEqual(0);
+});

e2e/call-done-twice/__tests__/index.test.js

@@ -0,0 +1,21 @@
+/**
+ * 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);
+  });
+});

e2e/call-done-twice/package.json

@@ -0,0 +1,5 @@
+{
+    "jest": {
+      "testEnvironment": "node"
+    }
+}

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');
+  });
+});

e2e/done-in-hooks/package.json

@@ -0,0 +1,5 @@
+{
+  "jest": {
+    "testEnvironment": "node"
+  }
+}

packages/jest-circus/src/eventHandler.ts

@@ -113,7 +113,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': {

packages/jest-circus/src/run.ts

@@ -149,6 +149,7 @@ const _callCircusHook = async ({
   testContext?: Circus.TestContext;
 }): Promise<void> => {
   await dispatch({hook, name: 'hook_start'});
+  hook.seenDone = false;
   const timeout = hook.timeout || getState().testTimeout;
 
   try {
@@ -167,6 +168,7 @@ const _callCircusTest = async (
   testContext: Circus.TestContext,
 ): Promise<void> => {
   await dispatch({name: 'test_fn_start', test});
+  test.seenDone = false;
   const timeout = test.timeout || getState().testTimeout;
   invariant(test.fn, `Tests with no 'fn' should have 'mode' set to 'skipped'`);
 

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,23 @@ export const callAsyncCircusFn = (
     // soon as `done` called.
     if (takesDoneCallback(fn)) {
       let returnedValue: unknown = undefined;
+
       const done = (reason?: Error | string): void => {
+        if (!completed && testOrHook.seenDone) {
+          let message =
+            'Expected done to be called once, but it was called multiple times.';
+
+          if (reason) {
+            message += ' Reason: ' + reason.toString();
+          }
+
+          throw new ErrorWithStack(message, done);
+        } else {
+          testOrHook.seenDone = true;
+        }
         // We need to keep a stack here before the promise tick
         const errorAtDone = new ErrorWithStack(undefined, done);
+
         // 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 +218,6 @@ export const callAsyncCircusFn = (
           }
 
           let errorAsErrorObject: Error;
-
           if (checkIsError(reason)) {
             errorAsErrorObject = reason;
           } else {

packages/jest-core/src/__tests__/SearchSource.test.ts

@@ -415,7 +415,7 @@ describe('SearchSource', () => {
     );
     const rootPath = path.join(rootDir, 'root.js');
 
-    beforeEach(done => {
+    beforeEach(async () => {
       const {options: config} = normalize(
         {
           haste: {
@@ -435,12 +435,11 @@ describe('SearchSource', () => {
         },
         {} as Config.Argv,
       );
-      Runtime.createContext(config, {maxWorkers, watchman: false}).then(
-        context => {
-          searchSource = new SearchSource(context);
-          done();
-        },
-      );
+      const context = await Runtime.createContext(config, {
+        maxWorkers,
+        watchman: false,
+      });
+      searchSource = new SearchSource(context);
     });
 
     it('makes sure a file is related to itself', () => {
@@ -477,7 +476,7 @@ describe('SearchSource', () => {
   });
 
   describe('findRelatedTestsFromPattern', () => {
-    beforeEach(done => {
+    beforeEach(async () => {
       const {options: config} = normalize(
         {
           moduleFileExtensions: ['js', 'jsx', 'foobar'],
@@ -487,12 +486,11 @@ describe('SearchSource', () => {
         },
         {} as Config.Argv,
       );
-      Runtime.createContext(config, {maxWorkers, watchman: false}).then(
-        context => {
-          searchSource = new SearchSource(context);
-          done();
-        },
-      );
+      const context = await Runtime.createContext(config, {
+        maxWorkers,
+        watchman: false,
+      });
+      searchSource = new SearchSource(context);
     });
 
     it('returns empty search result for empty input', () => {

packages/jest-types/src/Circus.ts

@@ -28,6 +28,7 @@ export type Hook = {
   fn: HookFn;
   type: HookType;
   parent: DescribeBlock;
+  seenDone: boolean;
   timeout: number | undefined | null;
 };
 
@@ -238,6 +239,7 @@ export type TestEntry = {
   parent: DescribeBlock;
   startedAt?: number | null;
   duration?: number | null;
+  seenDone: boolean;
   status?: TestStatus | null; // whether the test has been skipped or run already
   timeout?: number;
 };