feat(jest-circus): Fail tests if a test takes a done callback and have return values (#9129)

Author: SimenB

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Fixes
 
+- `[jest-circus]` [**BREAKING**] Fail tests if a test takes a done callback and have return values ([#9129](https://github.com/facebook/jest/pull/9129))
 - `[jest-config, jest-resolve]` [**BREAKING**] Remove support for `browser` field ([#9943](https://github.com/facebook/jest/pull/9943))
 
 ### Chore & Maintenance

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

@@ -0,0 +1,53 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`errors when a test both returns a promise and takes a callback 1`] = `
+FAIL __tests__/promise-and-callback.test.js
+  ✕ promise-returning test with callback
+  ✕ async test with callback
+  ✕ test done before return value
+
+  ● promise-returning test with callback
+
+    Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
+    Returned value: Promise {}
+
+       8 | 'use strict';
+       9 | 
+    > 10 | it('promise-returning test with callback', done => {
+         | ^
+      11 |   done();
+      12 | 
+      13 |   return Promise.resolve();
+
+      at Object.it (__tests__/promise-and-callback.test.js:10:1)
+
+  ● async test with callback
+
+    Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
+    Returned value: Promise {}
+
+      14 | });
+      15 | 
+    > 16 | it('async test with callback', async done => {
+         | ^
+      17 |   done();
+      18 | });
+      19 | 
+
+      at Object.it (__tests__/promise-and-callback.test.js:16:1)
+
+  ● test done before return value
+
+    Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
+    Returned value: "foobar"
+
+      18 | });
+      19 | 
+    > 20 | it('test done before return value', done => {
+         | ^
+      21 |   done();
+      22 | 
+      23 |   return 'foobar';
+
+      at Object.it (__tests__/promise-and-callback.test.js:20:1)
+`;

e2e/__tests__/asyncAndCallback.test.ts

@@ -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.
+ */
+
+import {skipSuiteOnJasmine} from '@jest/test-utils';
+import wrap from 'jest-snapshot-serializer-raw';
+import runJest from '../runJest';
+import {extractSummary} from '../Utils';
+
+skipSuiteOnJasmine();
+
+test('errors when a test both returns a promise and takes a callback', () => {
+  const result = runJest('promise-and-callback');
+
+  const {rest} = extractSummary(result.stderr);
+  expect(wrap(rest)).toMatchSnapshot();
+  expect(result.exitCode).toBe(1);
+});

e2e/promise-and-callback/__tests__/promise-and-callback.test.js

@@ -0,0 +1,24 @@
+/**
+ * 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.
+ */
+
+'use strict';
+
+it('promise-returning test with callback', done => {
+  done();
+
+  return Promise.resolve();
+});
+
+it('async test with callback', async done => {
+  done();
+});
+
+it('test done before return value', done => {
+  done();
+
+  return 'foobar';
+});

e2e/promise-and-callback/package.json

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

packages/jest-circus/package.json

@@ -16,6 +16,7 @@
     "@jest/types": "^25.5.0",
     "chalk": "^3.0.0",
     "co": "^4.6.0",
+    "dedent": "^0.7.0",
     "expect": "^25.5.0",
     "is-generator-fn": "^2.0.0",
     "jest-each": "^25.5.0",
@@ -34,6 +35,7 @@
     "@jest/test-utils": "^25.5.0",
     "@types/babel__traverse": "^7.0.4",
     "@types/co": "^4.6.0",
+    "@types/dedent": "^0.7.0",
     "@types/graceful-fs": "^4.1.3",
     "@types/stack-utils": "^1.0.1",
     "execa": "^3.2.0",

packages/jest-circus/src/run.ts

@@ -138,7 +138,10 @@ const _callCircusHook = async ({
   const timeout = hook.timeout || getState().testTimeout;
 
   try {
-    await callAsyncCircusFn(hook.fn, testContext, {isHook: true, timeout});
+    await callAsyncCircusFn(hook.fn, testContext, hook.asyncError, {
+    isHook: true,
+    timeout,
+  });
     await dispatch({describeBlock, hook, name: 'hook_success', test});
   } catch (error) {
     await dispatch({describeBlock, error, hook, name: 'hook_failure', test});
@@ -158,7 +161,10 @@ const _callCircusTest = async (
   }
 
   try {
-    await callAsyncCircusFn(test.fn, testContext, {isHook: false, timeout});
+    await callAsyncCircusFn(test.fn, testContext, test.asyncError, {
+    isHook: false,
+    timeout,
+  });
     await dispatch({name: 'test_fn_success', test});
   } catch (error) {
     await dispatch({error, name: 'test_fn_failure', test});

packages/jest-circus/src/utils.ts

@@ -9,6 +9,7 @@ import type {Circus} from '@jest/types';
 import {convertDescriptorToString} from 'jest-util';
 import isGeneratorFn from 'is-generator-fn';
 import co from 'co';
+import dedent = require('dedent');
 import StackUtils = require('stack-utils');
 import prettyFormat = require('pretty-format');
 import {getState} from './state';
@@ -153,6 +154,7 @@ function checkIsError(error: any): error is Error {
 export const callAsyncCircusFn = (
   fn: Circus.AsyncFn,
   testContext: Circus.TestContext | undefined,
+  asyncError: Circus.Exception,
   {isHook, timeout}: {isHook?: boolean | null; timeout: number},
 ): Promise<any> => {
   let timeoutID: NodeJS.Timeout;
@@ -167,24 +169,47 @@ export const callAsyncCircusFn = (
     // If this fn accepts `done` callback we return a promise that fulfills as
     // soon as `done` called.
     if (fn.length) {
+      let returnedValue: unknown = undefined;
       const done = (reason?: Error | string): void => {
-        const errorAsErrorObject = checkIsError(reason)
-          ? reason
-          : new Error(`Failed: ${prettyFormat(reason, {maxDepth: 3})}`);
-
-        // Consider always throwing, regardless if `reason` is set or not
-        if (completed && reason) {
-          errorAsErrorObject.message =
-            'Caught error after test environment was torn down\n\n' +
-            errorAsErrorObject.message;
-
-          throw errorAsErrorObject;
-        }
-
-        return reason ? reject(errorAsErrorObject) : resolve();
+        // We need to keep a stack here before the promise tick
+        const errorAtDone = new Error();
+        // 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) {
+            asyncError.message = dedent`
+      Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
+      Returned value: ${prettyFormat(returnedValue, {maxDepth: 3})}
+      `;
+            return reject(asyncError);
+          }
+
+          let errorAsErrorObject: Error;
+
+          if (checkIsError(reason)) {
+            errorAsErrorObject = reason;
+          } else {
+            errorAsErrorObject = errorAtDone;
+            errorAtDone.message = `Failed: ${prettyFormat(reason, {
+              maxDepth: 3,
+            })}`;
+          }
+
+          // Consider always throwing, regardless if `reason` is set or not
+          if (completed && reason) {
+            errorAsErrorObject.message =
+              'Caught error after test environment was torn down\n\n' +
+              errorAsErrorObject.message;
+
+            throw errorAsErrorObject;
+          }
+
+          return reason ? reject(errorAsErrorObject) : resolve();
+        });
       };
 
-      return fn.call(testContext, done);
+      returnedValue = fn.call(testContext, done);
+
+      return;
     }
 
     let returnedValue;
@@ -194,7 +219,8 @@ export const callAsyncCircusFn = (
       try {
         returnedValue = fn.call(testContext);
       } catch (error) {
-        return reject(error);
+        reject(error);
+        return;
       }
     }
 
@@ -205,23 +231,25 @@ export const callAsyncCircusFn = (
       returnedValue !== null &&
       typeof returnedValue.then === 'function'
     ) {
-      return returnedValue.then(resolve, reject);
+      returnedValue.then(resolve, reject);
+      return;
     }
 
-    if (!isHook && returnedValue !== void 0) {
-      return reject(
+    if (!isHook && returnedValue !== undefined) {
+      reject(
         new Error(
-          `
+          dedent`
       test functions can only return Promise or undefined.
-      Returned value: ${String(returnedValue)}
+      Returned value: ${prettyFormat(returnedValue, {maxDepth: 3})}
       `,
         ),
       );
+      return;
     }
 
     // Otherwise this test is synchronous, and if it didn't throw it means
     // it passed.
-    return resolve();
+    resolve();
   })
     .then(() => {
       completed = true;