feat(jest-circus): Fail tests if a test takes a done callback and hav…

…e 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,59 @@
+// 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/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

@@ -145,6 +145,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 +160,37 @@ 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();
+        // 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 = `
+      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})}
+      `;
+            reject(asyncError);
+          }
+          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();
+        });
       };
 
-      return fn.call(testContext, done);
+      returnedValue = fn.call(testContext, done);
+
+      return;
     }
 
     let returnedValue;
@@ -186,7 +200,8 @@ export const callAsyncCircusFn = (
       try {
         returnedValue = fn.call(testContext);
       } catch (error) {
-        return reject(error);
+        reject(error);
+        return;
       }
     }
 
@@ -197,23 +212,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(
           `
       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;