diff --git a/CHANGELOG.md b/CHANGELOG.md index 91ec3a7baa3a..cb3d544c312d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap new file mode 100644 index 000000000000..66a2dc6288c4 --- /dev/null +++ b/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) +`; diff --git a/e2e/__tests__/asyncAndCallback.test.ts b/e2e/__tests__/asyncAndCallback.test.ts new file mode 100644 index 000000000000..81112af6ed72 --- /dev/null +++ b/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); +}); diff --git a/e2e/promise-and-callback/__tests__/promise-and-callback.test.js b/e2e/promise-and-callback/__tests__/promise-and-callback.test.js new file mode 100644 index 000000000000..042638c7ebb2 --- /dev/null +++ b/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'; +}); diff --git a/e2e/promise-and-callback/package.json b/e2e/promise-and-callback/package.json new file mode 100644 index 000000000000..148788b25446 --- /dev/null +++ b/e2e/promise-and-callback/package.json @@ -0,0 +1,5 @@ +{ + "jest": { + "testEnvironment": "node" + } +} diff --git a/packages/jest-circus/package.json b/packages/jest-circus/package.json index 4936defbf169..8298ec8f8646 100644 --- a/packages/jest-circus/package.json +++ b/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", diff --git a/packages/jest-circus/src/run.ts b/packages/jest-circus/src/run.ts index eabd09411a44..9df9df8979d9 100644 --- a/packages/jest-circus/src/run.ts +++ b/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}); diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index fa477c12d9db..fdec52805fc2 100644 --- a/packages/jest-circus/src/utils.ts +++ b/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 => { 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;