From aaa97d22dc7481b583c09fe7a6145428d1b20aed Mon Sep 17 00:00:00 2001 From: Simen Bekkhus Date: Sun, 13 Sep 2020 09:41:22 +0200 Subject: [PATCH] feat(globals): make test return values stricter --- CHANGELOG.md | 2 ++ .../jestAdapterInit.ts | 4 +-- packages/jest-circus/src/utils.ts | 8 +++-- .../jest-jasmine2/src/jasmineAsyncInstall.ts | 2 +- packages/jest-types/src/Global.ts | 14 ++++---- test-types/top-level-globals.test.ts | 33 ++++++++++++++++++- 6 files changed, 51 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dbbbe9f4a20f..c1e73282d6fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - `[jest-circus, jest-config, jest-runtime]` Add new `injectGlobals` config and CLI option to disable injecting global variables into the runtime ([#10484](https://github.com/facebook/jest/pull/10484)) - `[jest-each]` Fixes `.each` type to always be callable ([#10447](https://github.com/facebook/jest/pull/10447)) +- `[jest-globals]` [**BREAKING**] Disallow return values other than a `Promise` from hooks and tests +- `[jest-globals]` [**BREAKING**] Disallow mixing a done callback and returning a `Promise` from hooks and tests ### Fixes diff --git a/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts b/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts index 96651b7337d0..5e9e335a0b87 100644 --- a/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts +++ b/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts @@ -81,7 +81,7 @@ export const initialize = async ({ globalsObject.test.concurrent = (test => { const concurrent = ( testName: string, - testFn: () => Promise, + testFn: Global.ConcurrentTestFn, timeout?: number, ) => { // For concurrent tests we first run the function that returns promise, and then register a @@ -96,7 +96,7 @@ export const initialize = async ({ const only = ( testName: string, - testFn: () => Promise, + testFn: Global.ConcurrentTestFn, timeout?: number, ) => { const promise = mutex(() => testFn()); diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index 82b4b78e52d4..06046c41ebc1 100644 --- a/packages/jest-circus/src/utils.ts +++ b/packages/jest-circus/src/utils.ts @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. */ -import type {Circus} from '@jest/types'; +import type {Circus, Global} from '@jest/types'; import {convertDescriptorToString, formatTime} from 'jest-util'; import isGeneratorFn from 'is-generator-fn'; import co from 'co'; @@ -17,6 +17,10 @@ import {ROOT_DESCRIBE_BLOCK_NAME, getState} from './state'; const stackUtils = new StackUtils({cwd: 'A path that does not exist'}); +function takesDoneCallback(fn: Circus.AsyncFn): fn is Global.DoneTakingTestFn { + return fn.length > 0; +} + export const makeDescribe = ( name: Circus.BlockName, parent?: Circus.DescribeBlock, @@ -172,7 +176,7 @@ export const callAsyncCircusFn = ( // If this fn accepts `done` callback we return a promise that fulfills as // soon as `done` called. - if (fn.length) { + if (takesDoneCallback(fn)) { let returnedValue: unknown = undefined; const done = (reason?: Error | string): void => { // We need to keep a stack here before the promise tick diff --git a/packages/jest-jasmine2/src/jasmineAsyncInstall.ts b/packages/jest-jasmine2/src/jasmineAsyncInstall.ts index db15f338ed75..412d47919851 100644 --- a/packages/jest-jasmine2/src/jasmineAsyncInstall.ts +++ b/packages/jest-jasmine2/src/jasmineAsyncInstall.ts @@ -165,7 +165,7 @@ function makeConcurrent( ): Global.ItConcurrentBase { const concurrentFn = function ( specName: string, - fn: Global.TestFn, + fn: Global.ConcurrentTestFn, timeout?: number, ) { let promise: Promise = Promise.resolve(); diff --git a/packages/jest-types/src/Global.ts b/packages/jest-types/src/Global.ts index 411e61191d23..d017f40b0d6d 100644 --- a/packages/jest-types/src/Global.ts +++ b/packages/jest-types/src/Global.ts @@ -7,14 +7,16 @@ import type {CoverageMapData} from 'istanbul-lib-coverage'; +type ValidTestReturnValues = void | undefined; +type TestReturnValuePromise = Promise; +type TestReturnValue = ValidTestReturnValues | TestReturnValuePromise; + export type DoneFn = (reason?: string | Error) => void; +export type DoneTakingTestFn = (done: DoneFn) => ValidTestReturnValues; +export type PromiseReturningTestFn = () => TestReturnValue; export type TestName = string; -export type TestFn = ( - done?: DoneFn, -) => Promise | void | undefined; -export type ConcurrentTestFn = ( - done?: DoneFn, -) => Promise; +export type TestFn = PromiseReturningTestFn | DoneTakingTestFn; +export type ConcurrentTestFn = () => TestReturnValuePromise; export type BlockFn = () => void; export type BlockName = string; export type HookFn = TestFn; diff --git a/test-types/top-level-globals.test.ts b/test-types/top-level-globals.test.ts index 9b6499411362..971e68d88577 100644 --- a/test-types/top-level-globals.test.ts +++ b/test-types/top-level-globals.test.ts @@ -7,7 +7,7 @@ * @type ./empty.d.ts */ -import {expectType} from 'mlh-tsd'; +import {expectError, expectType} from 'mlh-tsd'; import { afterAll, afterEach, @@ -17,8 +17,12 @@ import { test, //eslint-disable-next-line import/no-extraneous-dependencies } from '@jest/globals'; +import type {Global} from '@jest/types'; const fn = () => {}; +const doneFn: Global.DoneTakingTestFn = done => { + done(); +}; const asyncFn = async () => {}; const timeout = 5; const testName = 'Test name'; @@ -28,15 +32,42 @@ const testTable = [[1, 2]]; expectType(afterAll(fn)); expectType(afterAll(asyncFn)); expectType(afterAll(fn, timeout)); +expectType(afterAll(asyncFn, timeout)); expectType(afterEach(fn)); expectType(afterEach(asyncFn)); expectType(afterEach(fn, timeout)); +expectType(afterEach(asyncFn, timeout)); expectType(beforeAll(fn)); expectType(beforeAll(asyncFn)); expectType(beforeAll(fn, timeout)); +expectType(beforeAll(asyncFn, timeout)); expectType(beforeEach(fn)); expectType(beforeEach(asyncFn)); expectType(beforeEach(fn, timeout)); +expectType(beforeEach(asyncFn, timeout)); + +expectType(test(testName, fn)); +expectType(test(testName, asyncFn)); +expectType(test(testName, doneFn)); +expectType(test(testName, fn, timeout)); +expectType(test(testName, asyncFn, timeout)); +expectType(test(testName, doneFn, timeout)); + +// wrong arguments +expectError(test(testName)); +expectError(test(testName, timeout)); +expectError(test(timeout, fn)); + +// wrong return value +expectError(test(testName, () => 42)); +expectError(test(testName, async () => 42)); + +// mixing done callback and promise +expectError( + test(testName, async done => { + done(); + }), +); expectType(test.each(testTable)(testName, fn)); expectType(test.each(testTable)(testName, fn, timeout));