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

CHANGELOG.md

@@ -7,6 +7,7 @@
 - `[babel-preset-jest]` Add `@babel/plugin-syntax-bigint` ([#8382](https://github.com/facebook/jest/pull/8382))
 - `[expect]` Add `BigInt` support to `toBeGreaterThan`, `toBeGreaterThanOrEqual`, `toBeLessThan` and `toBeLessThanOrEqual` ([#8382](https://github.com/facebook/jest/pull/8382))
 - `[expect, jest-matcher-utils]` Display change counts in annotation lines ([#9035](https://github.com/facebook/jest/pull/9035))
+- `[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]` Throw the full error message and stack when a Jest preset is missing a dependency ([#8924](https://github.com/facebook/jest/pull/8924))
 - `[jest-config]` [**BREAKING**] Set default display name color based on runner ([#8689](https://github.com/facebook/jest/pull/8689))
 - `[jest-config]` Merge preset globals with project globals ([#9027](https://github.com/facebook/jest/pull/9027))

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 take a 'done' callback and return anything. Make sure to _only_ use a callback or promises - not both
+    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 take a 'done' callback and return anything. Make sure to _only_ use a callback or promises - not both
+    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 take a 'done' callback and return anything. Make sure to _only_ use a callback or promises - not both
+    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": "^24.9.0",
     "chalk": "^2.0.1",
     "co": "^4.6.0",
+    "dedent": "^0.7.0",
     "expect": "^24.9.0",
     "is-generator-fn": "^2.0.0",
     "jest-each": "^24.9.0",
@@ -31,6 +32,7 @@
     "@jest/test-utils": "^24.4.0",
     "@types/babel__traverse": "^7.0.4",
     "@types/co": "^4.6.0",
+    "@types/dedent": "^0.7.0",
     "@types/stack-utils": "^1.0.1",
     "execa": "^2.0.4",
     "jest-runtime": "^24.9.0"

packages/jest-circus/src/run.ts

@@ -135,7 +135,10 @@ const _callCircusHook = ({
 }): Promise<unknown> => {
   dispatch({hook, name: 'hook_start'});
   const timeout = hook.timeout || getState().testTimeout;
-  return callAsyncCircusFn(hook.fn, testContext, {isHook: true, timeout})
+  return callAsyncCircusFn(hook.fn, testContext, hook.asyncError, {
+    isHook: true,
+    timeout,
+  })
     .then(() => dispatch({describeBlock, hook, name: 'hook_success', test}))
     .catch(error =>
       dispatch({describeBlock, error, hook, name: 'hook_failure', test}),
@@ -155,7 +158,10 @@ const _callCircusTest = (
     return Promise.resolve();
   }
 
-  return callAsyncCircusFn(test.fn!, testContext, {isHook: false, timeout})
+  return callAsyncCircusFn(test.fn!, testContext, test.asyncError, {
+    isHook: false,
+    timeout,
+  })
     .then(() => dispatch({name: 'test_fn_success', test}))
     .catch(error => dispatch({error, name: 'test_fn_failure', test}));
 };

packages/jest-circus/src/utils.ts

@@ -9,6 +9,7 @@ import {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';
@@ -145,6 +146,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;
@@ -159,24 +161,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 take a 'done' callback and return anything. Make sure to _only_ use a callback or promises - not both
+      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;
@@ -186,7 +211,8 @@ export const callAsyncCircusFn = (
       try {
         returnedValue = fn.call(testContext);
       } catch (error) {
-        return reject(error);
+        reject(error);
+        return;
       }
     }
 
@@ -197,23 +223,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;

yarn.lock

@@ -1887,6 +1887,11 @@
   resolved "https://registry.yarnpkg.com/@types/convert-source-map/-/convert-source-map-1.5.1.tgz#d4d180dd6adc5cb68ad99bd56e03d637881f4616"
   integrity sha512-laiDIXqqthjJlyAMYAXOtN3N8+UlbM+KvZi4BaY5ZOekmVkBs/UxfK5O0HWeJVG2eW8F+Mu2ww13fTX+kY1FlQ==
 
+"@types/dedent@^0.7.0":
+  version "0.7.0"
+  resolved "https://registry.yarnpkg.com/@types/dedent/-/dedent-0.7.0.tgz#155f339ca404e6dd90b9ce46a3f78fd69ca9b050"
+  integrity sha512-EGlKlgMhnLt/cM4DbUSafFdrkeJoC9Mvnj0PUCU7tFmTjMjNRT957kXCx0wYm3JuEq4o4ZsS5vG+NlkM2DMd2A==
+
 "@types/eslint-visitor-keys@^1.0.0":
   version "1.0.0"
   resolved "https://registry.yarnpkg.com/@types/eslint-visitor-keys/-/eslint-visitor-keys-1.0.0.tgz#1ee30d79544ca84d68d4b3cdb0af4f205663dd2d"