From 621c8e61c3e5321a369b18fc9b818bf9b9f89918 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 Nov 2019 16:49:55 +0100 Subject: [PATCH] feat(jest-circus): Fail tests if a test takes a done callback and have return values --- CHANGELOG.md | 1 + .../asyncAndCallback.test.ts.snap | 59 +++++++++++++++++++ e2e/__tests__/asyncAndCallback.test.ts | 21 +++++++ .../__tests__/promise-and-callback.test.js | 24 ++++++++ e2e/promise-and-callback/package.json | 5 ++ packages/jest-circus/src/run.ts | 10 +++- packages/jest-circus/src/utils.ts | 59 ++++++++++++------- 7 files changed, 156 insertions(+), 23 deletions(-) create mode 100644 e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap create mode 100644 e2e/__tests__/asyncAndCallback.test.ts create mode 100644 e2e/promise-and-callback/__tests__/promise-and-callback.test.js create mode 100644 e2e/promise-and-callback/package.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 975de612d35c..2288935331c8 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap new file mode 100644 index 000000000000..924f0de96c72 --- /dev/null +++ b/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) +`; 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/src/run.ts b/packages/jest-circus/src/run.ts index 5f99ac2ccb24..a67fce5a65b7 100644 --- a/packages/jest-circus/src/run.ts +++ b/packages/jest-circus/src/run.ts @@ -135,7 +135,10 @@ const _callCircusHook = ({ }): Promise => { 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})); }; diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index 284239484594..d0d940adb95c 100644 --- a/packages/jest-circus/src/utils.ts +++ b/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 => { 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;