From d9f6aafa114582f7348beb9558df99552764989a Mon Sep 17 00:00:00 2001 From: Dmitri Date: Tue, 26 Apr 2022 12:07:06 +0300 Subject: [PATCH] fix(jest-circus): improve test.concurrent (#12748) --- CHANGELOG.md | 2 + e2e/__tests__/jasmineAsync.test.ts | 16 ++++++- .../__tests__/concurrent.test.js | 28 +++++++++-- .../concurrentWithinDescribe.test.js | 10 +++- packages/jest-circus/src/eventHandler.ts | 3 +- packages/jest-circus/src/index.ts | 25 ++++++++-- .../jestAdapterInit.ts | 44 +----------------- packages/jest-circus/src/run.ts | 46 ++++++++++++++++++- packages/jest-circus/src/state.ts | 1 + packages/jest-circus/src/utils.ts | 7 +++ packages/jest-types/src/Circus.ts | 4 ++ 11 files changed, 129 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2083f6337a08..66d0b68cddb7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ ### Fixes +- `[jest-circus]` Improve `test.concurrent` ([#12748](https://github.com/facebook/jest/pull/12748)) + ### Chore & Maintenance - `[jest-serializer]` Remove deprecated module from source tree ([#12735](https://github.com/facebook/jest/pull/12735)) diff --git a/e2e/__tests__/jasmineAsync.test.ts b/e2e/__tests__/jasmineAsync.test.ts index 8b6cfb93ec1c..3d716e3171fb 100644 --- a/e2e/__tests__/jasmineAsync.test.ts +++ b/e2e/__tests__/jasmineAsync.test.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import {isJestJasmineRun} from '@jest/test-utils'; import runJest, {json as runWithJson} from '../runJest'; describe('async jasmine', () => { @@ -107,16 +108,25 @@ describe('async jasmine', () => { }); it('works with concurrent', () => { - const {json} = runWithJson('jasmine-async', ['concurrent.test.js']); + const {json, stderr} = runWithJson('jasmine-async', ['concurrent.test.js']); expect(json.numTotalTests).toBe(4); expect(json.numPassedTests).toBe(2); expect(json.numFailedTests).toBe(1); expect(json.numPendingTests).toBe(1); expect(json.testResults[0].message).toMatch(/concurrent test fails/); + if (!isJestJasmineRun()) { + expect(stderr.match(/\[\[\w+\]\]/g)).toEqual([ + '[[beforeAll]]', + '[[test]]', + '[[test]]', + '[[test]]', + '[[afterAll]]', + ]); + } }); it('works with concurrent within a describe block when invoked with testNamePattern', () => { - const {json} = runWithJson('jasmine-async', [ + const {json, stderr} = runWithJson('jasmine-async', [ '--testNamePattern', 'one concurrent test fails', 'concurrentWithinDescribe.test.js', @@ -126,6 +136,8 @@ describe('async jasmine', () => { expect(json.numFailedTests).toBe(1); expect(json.numPendingTests).toBe(1); expect(json.testResults[0].message).toMatch(/concurrent test fails/); + expect(stderr).toMatch(/this is logged \d/); + expect(stderr).not.toMatch(/this is not logged \d/); }); it('works with concurrent.each', () => { diff --git a/e2e/jasmine-async/__tests__/concurrent.test.js b/e2e/jasmine-async/__tests__/concurrent.test.js index ef0cc1ba7b10..2b852c2f141e 100644 --- a/e2e/jasmine-async/__tests__/concurrent.test.js +++ b/e2e/jasmine-async/__tests__/concurrent.test.js @@ -7,7 +7,27 @@ 'use strict'; -it.concurrent('one', () => Promise.resolve()); -it.concurrent.skip('two', () => Promise.resolve()); -it.concurrent('three', () => Promise.resolve()); -it.concurrent('concurrent test fails', () => Promise.reject()); +const marker = s => console.log(`[[${s}]]`); + +beforeAll(() => marker('beforeAll')); +afterAll(() => marker('afterAll')); + +beforeEach(() => marker('beforeEach')); +afterEach(() => marker('afterEach')); + +it.concurrent('one', () => { + marker('test'); + return Promise.resolve(); +}); +it.concurrent.skip('two', () => { + marker('test'); + return Promise.resolve(); +}); +it.concurrent('three', () => { + marker('test'); + return Promise.resolve(); +}); +it.concurrent('concurrent test fails', () => { + marker('test'); + return Promise.reject(); +}); diff --git a/e2e/jasmine-async/__tests__/concurrentWithinDescribe.test.js b/e2e/jasmine-async/__tests__/concurrentWithinDescribe.test.js index 1e674e10d2e1..56c389d3a089 100644 --- a/e2e/jasmine-async/__tests__/concurrentWithinDescribe.test.js +++ b/e2e/jasmine-async/__tests__/concurrentWithinDescribe.test.js @@ -8,6 +8,12 @@ 'use strict'; describe('one', () => { - it.concurrent('concurrent test gets skipped', () => Promise.resolve()); - it.concurrent('concurrent test fails', () => Promise.reject()); + it.concurrent('concurrent test gets skipped', () => { + console.log(`this is not logged ${Math.random()}`); + return Promise.resolve(); + }); + it.concurrent('concurrent test fails', () => { + console.log(`this is logged ${Math.random()}`); + return Promise.reject(new Error()); + }); }); diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts index 19febb73055e..21c9b35ab2ba 100644 --- a/packages/jest-circus/src/eventHandler.ts +++ b/packages/jest-circus/src/eventHandler.ts @@ -122,7 +122,7 @@ const eventHandler: Circus.EventHandler = (event, state) => { } case 'add_test': { const {currentDescribeBlock, currentlyRunningTest, hasStarted} = state; - const {asyncError, fn, mode, testName: name, timeout} = event; + const {asyncError, fn, mode, testName: name, timeout, concurrent} = event; if (currentlyRunningTest) { currentlyRunningTest.errors.push( @@ -143,6 +143,7 @@ const eventHandler: Circus.EventHandler = (event, state) => { const test = makeTest( fn, mode, + concurrent, name, currentDescribeBlock, timeout, diff --git a/packages/jest-circus/src/index.ts b/packages/jest-circus/src/index.ts index 33c4e43dec50..cc8c63872cbe 100644 --- a/packages/jest-circus/src/index.ts +++ b/packages/jest-circus/src/index.ts @@ -117,17 +117,27 @@ const test: Global.It = (() => { testName: Circus.TestNameLike, fn: Circus.TestFn, timeout?: number, - ): void => _addTest(testName, undefined, fn, test, timeout); + ): void => _addTest(testName, undefined, false, fn, test, timeout); const skip = ( testName: Circus.TestNameLike, fn?: Circus.TestFn, timeout?: number, - ): void => _addTest(testName, 'skip', fn, skip, timeout); + ): void => _addTest(testName, 'skip', false, fn, skip, timeout); const only = ( testName: Circus.TestNameLike, fn: Circus.TestFn, timeout?: number, - ): void => _addTest(testName, 'only', fn, test.only, timeout); + ): void => _addTest(testName, 'only', false, fn, test.only, timeout); + const concurrentTest = ( + testName: Circus.TestNameLike, + fn: Circus.TestFn, + timeout?: number, + ): void => _addTest(testName, undefined, true, fn, concurrentTest, timeout); + const concurrentOnly = ( + testName: Circus.TestNameLike, + fn: Circus.TestFn, + timeout?: number, + ): void => _addTest(testName, 'only', true, fn, concurrentOnly, timeout); test.todo = (testName: Circus.TestNameLike, ...rest: Array): void => { if (rest.length > 0 || typeof testName !== 'string') { @@ -136,12 +146,13 @@ const test: Global.It = (() => { test.todo, ); } - return _addTest(testName, 'todo', () => {}, test.todo); + return _addTest(testName, 'todo', false, () => {}, test.todo); }; const _addTest = ( testName: Circus.TestNameLike, mode: Circus.TestMode, + concurrent: boolean, fn: Circus.TestFn | undefined, testFn: ( testName: Circus.TestNameLike, @@ -173,6 +184,7 @@ const test: Global.It = (() => { return dispatchSync({ asyncError, + concurrent, fn, mode, name: 'add_test', @@ -184,9 +196,14 @@ const test: Global.It = (() => { test.each = bindEach(test); only.each = bindEach(only); skip.each = bindEach(skip); + concurrentTest.each = bindEach(concurrentTest, false); + concurrentOnly.each = bindEach(concurrentOnly, false); test.only = only; test.skip = skip; + test.concurrent = concurrentTest; + concurrentTest.only = concurrentOnly; + concurrentTest.skip = skip; return test; })(); 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 8a81880c748e..6e00f54eed5f 100644 --- a/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts +++ b/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.ts @@ -5,7 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import throat from 'throat'; import type {JestEnvironment} from '@jest/environment'; import {JestExpect, jestExpect} from '@jest/expect'; import { @@ -16,7 +15,6 @@ import { createEmptyTestResult, } from '@jest/test-result'; import type {Circus, Config, Global} from '@jest/types'; -import {bind} from 'jest-each'; import {formatExecError, formatResultsErrors} from 'jest-message-util'; import { SnapshotState, @@ -63,8 +61,7 @@ export const initialize = async ({ if (globalConfig.testTimeout) { getRunnerState().testTimeout = globalConfig.testTimeout; } - - const mutex = throat(globalConfig.maxConcurrency); + getRunnerState().maxConcurrency = globalConfig.maxConcurrency; // @ts-expect-error const globalsObject: Global.TestFrameworkGlobals = { @@ -76,45 +73,6 @@ export const initialize = async ({ xtest: globals.it.skip, }; - globalsObject.test.concurrent = (test => { - const concurrent = ( - testName: Global.TestNameLike, - testFn: Global.ConcurrentTestFn, - timeout?: number, - ) => { - // For concurrent tests we first run the function that returns promise, and then register a - // normal test that will be waiting on the returned promise (when we start the test, the promise - // will already be in the process of execution). - // Unfortunately at this stage there's no way to know if there are any `.only` tests in the suite - // that will result in this test to be skipped, so we'll be executing the promise function anyway, - // even if it ends up being skipped. - const promise = mutex(() => testFn()); - // Avoid triggering the uncaught promise rejection handler in case the test errors before - // being awaited on. - promise.catch(() => {}); - globalsObject.test(testName, () => promise, timeout); - }; - - const only = ( - testName: Global.TestNameLike, - testFn: Global.ConcurrentTestFn, - timeout?: number, - ) => { - const promise = mutex(() => testFn()); - // eslint-disable-next-line jest/no-focused-tests - test.only(testName, () => promise, timeout); - }; - - concurrent.only = only; - concurrent.skip = test.skip; - - concurrent.each = bind(test, false); - concurrent.skip.each = bind(test.skip, false); - only.each = bind(test.only, false); - - return concurrent; - })(globalsObject.test); - addEventHandler(eventHandler); if (environment.handleTestEvent) { diff --git a/packages/jest-circus/src/run.ts b/packages/jest-circus/src/run.ts index 9550629ecf5d..f22faea266ea 100644 --- a/packages/jest-circus/src/run.ts +++ b/packages/jest-circus/src/run.ts @@ -5,6 +5,7 @@ * LICENSE file in the root directory of this source tree. */ +import throat from 'throat'; import type {Circus} from '@jest/types'; import {dispatch, getState} from './state'; import {RETRY_TIMES} from './types'; @@ -20,7 +21,7 @@ import { const run = async (): Promise => { const {rootDescribeBlock} = getState(); await dispatch({name: 'run_start'}); - await _runTestsForDescribeBlock(rootDescribeBlock); + await _runTestsForDescribeBlock(rootDescribeBlock, true); await dispatch({name: 'run_finish'}); return makeRunResult( getState().rootDescribeBlock, @@ -30,6 +31,7 @@ const run = async (): Promise => { const _runTestsForDescribeBlock = async ( describeBlock: Circus.DescribeBlock, + isRootBlock = false, ) => { await dispatch({describeBlock, name: 'run_describe_start'}); const {beforeAll, afterAll} = getAllHooksForDescribe(describeBlock); @@ -42,6 +44,24 @@ const _runTestsForDescribeBlock = async ( } } + if (isRootBlock) { + const concurrentTests = collectConcurrentTests(describeBlock); + const mutex = throat(getState().maxConcurrency); + for (const test of concurrentTests) { + try { + const promise = mutex(test.fn); + // Avoid triggering the uncaught promise rejection handler in case the + // test errors before being awaited on. + promise.catch(() => {}); + test.fn = () => promise; + } catch (err) { + test.fn = () => { + throw err; + }; + } + } + } + // Tests that fail and are retried we run after other tests // eslint-disable-next-line no-restricted-globals const retryTimes = parseInt(global[RETRY_TIMES], 10) || 0; @@ -91,6 +111,30 @@ const _runTestsForDescribeBlock = async ( await dispatch({describeBlock, name: 'run_describe_finish'}); }; +function collectConcurrentTests( + describeBlock: Circus.DescribeBlock, +): Array & {fn: Circus.ConcurrentTestFn}> { + if (describeBlock.mode === 'skip') { + return []; + } + const {hasFocusedTests, testNamePattern} = getState(); + return describeBlock.children.flatMap(child => { + switch (child.type) { + case 'describeBlock': + return collectConcurrentTests(child); + case 'test': + const skip = + !child.concurrent || + child.mode === 'skip' || + (hasFocusedTests && child.mode !== 'only') || + (testNamePattern && !testNamePattern.test(getTestID(child))); + return skip + ? [] + : [child as Circus.TestEntry & {fn: Circus.ConcurrentTestFn}]; + } + }); +} + const _runTest = async ( test: Circus.TestEntry, parentSkipped: boolean, diff --git a/packages/jest-circus/src/state.ts b/packages/jest-circus/src/state.ts index 264e0d160746..e9f0321e979f 100644 --- a/packages/jest-circus/src/state.ts +++ b/packages/jest-circus/src/state.ts @@ -27,6 +27,7 @@ const createState = (): Circus.State => { hasFocusedTests: false, hasStarted: false, includeTestLocationInResult: false, + maxConcurrency: 5, parentProcess: null, rootDescribeBlock: ROOT_DESCRIBE_BLOCK, testNamePattern: null, diff --git a/packages/jest-circus/src/utils.ts b/packages/jest-circus/src/utils.ts index 98f08a06c9ec..11060646a3be 100644 --- a/packages/jest-circus/src/utils.ts +++ b/packages/jest-circus/src/utils.ts @@ -56,6 +56,7 @@ export const makeDescribe = ( export const makeTest = ( fn: Circus.TestFn, mode: Circus.TestMode, + concurrent: boolean, name: Circus.TestName, parent: Circus.DescribeBlock, timeout: number | undefined, @@ -63,6 +64,7 @@ export const makeTest = ( ): Circus.TestEntry => ({ type: 'test', // eslint-disable-next-line sort-keys asyncError, + concurrent, duration: null, errors: [], fn, @@ -128,6 +130,11 @@ type TestHooks = { export const getEachHooksForTest = (test: Circus.TestEntry): TestHooks => { const result: TestHooks = {afterEach: [], beforeEach: []}; + if (test.concurrent) { + // *Each hooks are not run for concurrent tests + return result; + } + let block: Circus.DescribeBlock | undefined | null = test.parent; do { diff --git a/packages/jest-types/src/Circus.ts b/packages/jest-types/src/Circus.ts index 4318d4f7cf2a..cf2306b12e20 100644 --- a/packages/jest-types/src/Circus.ts +++ b/packages/jest-types/src/Circus.ts @@ -18,6 +18,7 @@ export type TestMode = BlockMode; export type TestName = Global.TestName; export type TestNameLike = Global.TestNameLike; export type TestFn = Global.TestFn; +export type ConcurrentTestFn = Global.ConcurrentTestFn; export type HookFn = Global.HookFn; export type AsyncFn = TestFn | HookFn; export type SharedHookType = 'afterAll' | 'beforeAll'; @@ -71,6 +72,7 @@ export type SyncEvent = testName: TestName; fn: TestFn; mode?: TestMode; + concurrent: boolean; timeout: number | undefined; } | { @@ -216,6 +218,7 @@ export type State = { testTimeout: number; unhandledErrors: Array; includeTestLocationInResult: boolean; + maxConcurrency: number; }; export type DescribeBlock = { @@ -239,6 +242,7 @@ export type TestEntry = { fn: TestFn; invocations: number; mode: TestMode; + concurrent: boolean; name: TestName; parent: DescribeBlock; startedAt?: number | null;