feat(globals): make test return values stricter (#10512)

CHANGELOG.md

@@ -20,6 +20,8 @@
 - `[jest-circus]` Fix `testLocation` on Windows when using `test.each` ([#10871](https://github.com/facebook/jest/pull/10871))
 - `[jest-console]` `console.dir` now respects the second argument correctly ([#10638](https://github.com/facebook/jest/pull/10638))
 - `[jest-environment-jsdom]` Use inner realm’s `ArrayBuffer` constructor ([#10885](https://github.com/facebook/jest/pull/10885))
+- `[jest-globals]` [**BREAKING**] Disallow return values other than a `Promise` from hooks and tests ([#10512](https://github.com/facebook/jest/pull/10512))
+- `[jest-globals]` [**BREAKING**] Disallow mixing a done callback and returning a `Promise` from hooks and tests ([#10512](https://github.com/facebook/jest/pull/10512))
 - `[jest-haste-map]` Vendor `NodeWatcher` from `sane` ([#10919](https://github.com/facebook/jest/pull/10919))
 - `[jest-jasmine2]` Fixed the issue of beforeAll & afterAll hooks getting executed even if it is inside a skipped `describe` block when it has child `tests` marked as either `only` or `todo` [#10451](https://github.com/facebook/jest/issues/10451)
 - `[jest-jasmine2]` Fixed the issues of child `tests` marked with `only` or `todo` getting executed even if it is inside a skipped parent `describe` block [#10451](https://github.com/facebook/jest/issues/10451)

packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts

@@ -83,7 +83,7 @@ export const initialize = async ({
   globalsObject.test.concurrent = (test => {
     const concurrent = (
       testName: string,
-      testFn: () => Promise<unknown>,
+      testFn: Global.ConcurrentTestFn,
       timeout?: number,
     ) => {
       // For concurrent tests we first run the function that returns promise, and then register a
@@ -98,7 +98,7 @@ export const initialize = async ({
 
     const only = (
       testName: string,
-      testFn: () => Promise<unknown>,
+      testFn: Global.ConcurrentTestFn,
       timeout?: number,
     ) => {
       const promise = mutex(() => testFn());

packages/jest-circus/src/utils.ts

@@ -12,7 +12,7 @@ import isGeneratorFn from 'is-generator-fn';
 import slash = require('slash');
 import StackUtils = require('stack-utils');
 import type {AssertionResult, Status} from '@jest/test-result';
-import type {Circus} from '@jest/types';
+import type {Circus, Global} from '@jest/types';
 import {ErrorWithStack, convertDescriptorToString, formatTime} from 'jest-util';
 import prettyFormat from 'pretty-format';
 import {ROOT_DESCRIBE_BLOCK_NAME, getState} from './state';
@@ -21,6 +21,16 @@ const stackUtils = new StackUtils({cwd: 'A path that does not exist'});
 
 const jestEachBuildDir = slash(path.dirname(require.resolve('jest-each')));
 
+function takesDoneCallback(fn: Circus.AsyncFn): fn is Global.DoneTakingTestFn {
+  return fn.length > 0;
+}
+
+function isGeneratorFunction(
+  fn: Global.PromiseReturningTestFn | Global.GeneratorReturningTestFn,
+): fn is Global.GeneratorReturningTestFn {
+  return isGeneratorFn(fn);
+}
+
 export const makeDescribe = (
   name: Circus.BlockName,
   parent?: Circus.DescribeBlock,
@@ -177,7 +187,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
@@ -216,25 +226,17 @@ export const callAsyncCircusFn = (
         });
       };
 
-      returnedValue = fn.call<
-        Circus.TestContext | undefined,
-        Array<typeof done>,
-        void | Promise<unknown> | Generator | undefined
-      >(testContext, done);
+      returnedValue = fn.call(testContext, done);
 
       return;
     }
 
-    let returnedValue: any;
-    if (isGeneratorFn(fn)) {
+    let returnedValue: Global.TestReturnValue;
+    if (isGeneratorFunction(fn)) {
       returnedValue = co.wrap(fn).call({});
     } else {
       try {
-        returnedValue = fn.call<
-          Circus.TestContext | undefined,
-          [],
-          void | Promise<unknown> | Generator | undefined
-        >(testContext);
+        returnedValue = fn.call(testContext);
       } catch (error) {
         reject(error);
         return;

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<unknown> = Promise.resolve();

packages/jest-types/src/Circus.ts

@@ -20,7 +20,7 @@ export type HookFn = Global.HookFn;
 export type AsyncFn = TestFn | HookFn;
 export type SharedHookType = 'afterAll' | 'beforeAll';
 export type HookType = SharedHookType | 'afterEach' | 'beforeEach';
-export type TestContext = Record<string, unknown>;
+export type TestContext = Global.TestContext;
 export type Exception = any; // Since in JS anything can be thrown as an error.
 export type FormattedError = string; // String representation of error.
 export type Hook = {

packages/jest-types/src/Global.ts

@@ -7,17 +7,32 @@
 
 import type {CoverageMapData} from 'istanbul-lib-coverage';
 
-export type GeneratorFn = (...args: Array<any>) => Generator;
+export type ValidTestReturnValues = void | undefined;
+type TestReturnValuePromise = Promise<unknown>;
+type TestReturnValueGenerator = Generator<void, unknown, void>;
+export type TestReturnValue = ValidTestReturnValues | TestReturnValuePromise;
+
+export type TestContext = Record<string, unknown>;
+
 export type DoneFn = (reason?: string | Error) => void;
-export type CallbackFn = (
-  done?: DoneFn,
-) => void | undefined | Promise<void | undefined | unknown>;
+// these should not be undefined
+export type DoneTakingTestFn = (
+  this: TestContext | undefined,
+  done: DoneFn,
+) => ValidTestReturnValues;
+export type PromiseReturningTestFn = (
+  this: TestContext | undefined,
+) => TestReturnValue;
+export type GeneratorReturningTestFn = (
+  this: TestContext | undefined,
+) => TestReturnValueGenerator;
 
 export type TestName = string;
-export type TestFn = GeneratorFn | CallbackFn;
-export type ConcurrentTestFn = (
-  done?: DoneFn,
-) => Promise<void | undefined | unknown>;
+export type TestFn =
+  | PromiseReturningTestFn
+  | GeneratorReturningTestFn
+  | DoneTakingTestFn;
+export type ConcurrentTestFn = () => TestReturnValuePromise;
 export type BlockFn = () => void;
 export type BlockName = string;
 export type HookFn = TestFn;

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,
@@ -16,8 +16,12 @@ import {
   describe,
   test,
 } from '@jest/globals';
+import type {Global} from '@jest/types';
 
 const fn = () => {};
+const doneFn: Global.DoneTakingTestFn = done => {
+  done();
+};
 const asyncFn = async () => {};
 const genFn = function* () {};
 const timeout = 5;
@@ -29,18 +33,55 @@ expectType<void>(afterAll(fn));
 expectType<void>(afterAll(asyncFn));
 expectType<void>(afterAll(genFn));
 expectType<void>(afterAll(fn, timeout));
+expectType<void>(afterAll(asyncFn, timeout));
+expectType<void>(afterAll(genFn, timeout));
 expectType<void>(afterEach(fn));
 expectType<void>(afterEach(asyncFn));
 expectType<void>(afterEach(genFn));
 expectType<void>(afterEach(fn, timeout));
+expectType<void>(afterEach(asyncFn, timeout));
+expectType<void>(afterEach(genFn, timeout));
 expectType<void>(beforeAll(fn));
 expectType<void>(beforeAll(asyncFn));
 expectType<void>(beforeAll(genFn));
 expectType<void>(beforeAll(fn, timeout));
+expectType<void>(beforeAll(asyncFn, timeout));
+expectType<void>(beforeAll(genFn, timeout));
 expectType<void>(beforeEach(fn));
 expectType<void>(beforeEach(asyncFn));
 expectType<void>(beforeEach(genFn));
 expectType<void>(beforeEach(fn, timeout));
+expectType<void>(beforeEach(asyncFn, timeout));
+expectType<void>(beforeEach(genFn, timeout));
+
+expectType<void>(test(testName, fn));
+expectType<void>(test(testName, asyncFn));
+expectType<void>(test(testName, doneFn));
+expectType<void>(test(testName, genFn));
+expectType<void>(test(testName, fn, timeout));
+expectType<void>(test(testName, asyncFn, timeout));
+expectType<void>(test(testName, doneFn, timeout));
+expectType<void>(test(testName, genFn, timeout));
+
+// wrong arguments
+expectError(test(testName));
+expectError(test(testName, timeout));
+expectError(test(timeout, fn));
+
+// wrong return value
+expectError(test(testName, () => 42));
+
+// mixing done callback and promise/generator
+expectError(
+  test(testName, async done => {
+    done();
+  }),
+);
+expectError(
+  test(testName, function* (done) {
+    done();
+  }),
+);
 
 expectType<void>(test(testName, fn));
 expectType<void>(test(testName, asyncFn));