From 621c8e61c3e5321a369b18fc9b818bf9b9f89918 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 Nov 2019 16:49:55 +0100 Subject: [PATCH 1/5] 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; From fb8f049640ef2f798ffeaff9ac9651c28ddf7f8f Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 Nov 2019 17:38:46 +0100 Subject: [PATCH 2/5] fix stack trace on done with args --- packages/jest-circus/src/utils.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index d0d940adb95c..5c7062939380 100644 --- a/packages/jest-circus/src/utils.ts +++ b/packages/jest-circus/src/utils.ts @@ -162,6 +162,8 @@ export const callAsyncCircusFn = ( if (fn.length) { let returnedValue: unknown = undefined; const done = (reason?: Error | string): void => { + // 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) { @@ -169,11 +171,19 @@ export const callAsyncCircusFn = ( 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); + return reject(asyncError); + } + + let errorAsErrorObject: Error; + + if (checkIsError(reason)) { + errorAsErrorObject = reason; + } else { + errorAsErrorObject = errorAtDone; + errorAtDone.message = `Failed: ${prettyFormat(reason, { + maxDepth: 3, + })}`; } - 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) { From badff00505a04a2b10983774019409ad39dc20ec Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Mon, 4 Nov 2019 17:48:28 +0100 Subject: [PATCH 3/5] use dedent --- .../asyncAndCallback.test.ts.snap | 18 ++++++------------ packages/jest-circus/package.json | 2 ++ packages/jest-circus/src/utils.ts | 5 +++-- yarn.lock | 5 +++++ 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap index 924f0de96c72..8c31a0084243 100644 --- a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap +++ b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap @@ -8,9 +8,8 @@ FAIL __tests__/promise-and-callback.test.js ● 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 {} + 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 | @@ -20,14 +19,12 @@ FAIL __tests__/promise-and-callback.test.js 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 {} + 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 | @@ -37,14 +34,12 @@ FAIL __tests__/promise-and-callback.test.js 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" + 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 | @@ -54,6 +49,5 @@ FAIL __tests__/promise-and-callback.test.js 22 | 23 | return 'foobar'; - at Object.it (__tests__/promise-and-callback.test.js:20:1) `; diff --git a/packages/jest-circus/package.json b/packages/jest-circus/package.json index d8484864aff8..2a394bb1aaa5 100644 --- a/packages/jest-circus/package.json +++ b/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" diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index 5c7062939380..66d8e572b8ea 100644 --- a/packages/jest-circus/src/utils.ts +++ b/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'; @@ -167,7 +168,7 @@ export const callAsyncCircusFn = ( // 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 = ` + 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})} `; @@ -229,7 +230,7 @@ export const callAsyncCircusFn = ( if (!isHook && returnedValue !== undefined) { reject( new Error( - ` + dedent` test functions can only return Promise or undefined. Returned value: ${prettyFormat(returnedValue, {maxDepth: 3})} `, diff --git a/yarn.lock b/yarn.lock index 39b8d123b7e8..d7ddd559f37e 100644 --- a/yarn.lock +++ b/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" From 91dbd0a528ad8d66704595167551219eb0cf0fa1 Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Sat, 2 May 2020 10:59:43 +0200 Subject: [PATCH 4/5] Update packages/jest-circus/src/utils.ts Co-authored-by: Tim Seckinger --- packages/jest-circus/src/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index 66d8e572b8ea..6912b34df9d6 100644 --- a/packages/jest-circus/src/utils.ts +++ b/packages/jest-circus/src/utils.ts @@ -169,7 +169,7 @@ export const callAsyncCircusFn = ( 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 + 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); From e956e2335d9f77807be6db2a75aa205949c0fa9a Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Sat, 2 May 2020 11:01:44 +0200 Subject: [PATCH 5/5] update message --- e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap index 8c31a0084243..66a2dc6288c4 100644 --- a/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap +++ b/e2e/__tests__/__snapshots__/asyncAndCallback.test.ts.snap @@ -8,7 +8,7 @@ FAIL __tests__/promise-and-callback.test.js ● 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 + 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'; @@ -23,7 +23,7 @@ FAIL __tests__/promise-and-callback.test.js ● 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 + Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise. Returned value: Promise {} 14 | }); @@ -38,7 +38,7 @@ FAIL __tests__/promise-and-callback.test.js ● 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 + Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise. Returned value: "foobar" 18 | });