From d59c2b996fb08e30b4d76a6bdaf39949e93489f3 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 28 Mar 2022 16:10:32 -0700 Subject: [PATCH] fix(parallel): minimize the number of beforeAll/afterAll hooks in parallel mode (#13138) Previously, we always formed groups consisting of a single test. Now, we group tests that share `beforeAll`/`afterAll` hooks into `config.workers` equally-sized groups. --- packages/playwright-test/src/runner.ts | 40 ++++++++++++++--- tests/playwright-test/test-parallel.spec.ts | 48 ++++++++++++++++++++- 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/packages/playwright-test/src/runner.ts b/packages/playwright-test/src/runner.ts index 1fa5e0313c..eb287e8ffb 100644 --- a/packages/playwright-test/src/runner.ts +++ b/packages/playwright-test/src/runner.ts @@ -315,7 +315,7 @@ export class Runner { fatalErrors.push(createNoTestsError()); // 8. Compute shards. - let testGroups = createTestGroups(rootSuite); + let testGroups = createTestGroups(rootSuite, config.workers); const shard = config.shard; if (shard) { @@ -621,7 +621,7 @@ function buildItemLocation(rootDir: string, testOrSuite: Suite | TestCase) { return `${path.relative(rootDir, testOrSuite.location.file)}:${testOrSuite.location.line}`; } -function createTestGroups(rootSuite: Suite): TestGroup[] { +function createTestGroups(rootSuite: Suite, workers: number): TestGroup[] { // This function groups tests that can be run together. // Tests cannot be run together when: // - They belong to different projects - requires different workers. @@ -632,7 +632,15 @@ function createTestGroups(rootSuite: Suite): TestGroup[] { // Using the map "workerHash -> requireFile -> group" makes us preserve the natural order // of worker hashes and require files for the simple cases. - const groups = new Map>(); + const groups = new Map>(); const createGroup = (test: TestCase): TestGroup => { return { @@ -656,18 +664,26 @@ function createTestGroups(rootSuite: Suite): TestGroup[] { withRequireFile = { general: createGroup(test), parallel: [], + parallelWithHooks: createGroup(test), }; withWorkerHash.set(test._requireFile, withRequireFile); } let insideParallel = false; - for (let parent: Suite | undefined = test.parent; parent; parent = parent.parent) + let hasAllHooks = false; + for (let parent: Suite | undefined = test.parent; parent; parent = parent.parent) { insideParallel = insideParallel || parent._parallelMode === 'parallel'; + hasAllHooks = hasAllHooks || parent._hooks.some(hook => hook.type === 'beforeAll' || hook.type === 'afterAll'); + } if (insideParallel) { - const group = createGroup(test); - group.tests.push(test); - withRequireFile.parallel.push(group); + if (hasAllHooks) { + withRequireFile.parallelWithHooks.tests.push(test); + } else { + const group = createGroup(test); + group.tests.push(test); + withRequireFile.parallel.push(group); + } } else { withRequireFile.general.tests.push(test); } @@ -680,6 +696,16 @@ function createTestGroups(rootSuite: Suite): TestGroup[] { if (withRequireFile.general.tests.length) result.push(withRequireFile.general); result.push(...withRequireFile.parallel); + + const parallelWithHooksGroupSize = Math.ceil(withRequireFile.parallelWithHooks.tests.length / workers); + let lastGroup: TestGroup | undefined; + for (const test of withRequireFile.parallelWithHooks.tests) { + if (!lastGroup || lastGroup.tests.length >= parallelWithHooksGroupSize) { + lastGroup = createGroup(test); + result.push(lastGroup); + } + lastGroup.tests.push(test); + } } } return result; diff --git a/tests/playwright-test/test-parallel.spec.ts b/tests/playwright-test/test-parallel.spec.ts index 0f9f7ccefe..c76a532de1 100644 --- a/tests/playwright-test/test-parallel.spec.ts +++ b/tests/playwright-test/test-parallel.spec.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { test, expect } from './playwright-test-fixtures'; +import { test, expect, countTimes, stripAnsi } from './playwright-test-fixtures'; test('test.describe.parallel should throw inside test.describe.serial', async ({ runInlineTest }) => { const result = await runInlineTest({ @@ -176,3 +176,49 @@ test('project.fullyParallel should work', async ({ runInlineTest }) => { expect(result.output).toContain('%% worker=1'); expect(result.output).toContain('%% worker=2'); }); + +test('parallel mode should minimize running beforeAll/afterAll hooks', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + const { test } = pwt; + test.describe.configure({ mode: 'parallel' }); + test.beforeAll(() => { + console.log('\\n%%beforeAll'); + }); + test.afterAll(() => { + console.log('\\n%%afterAll'); + }); + test('test1', () => {}); + test('test2', () => {}); + test('test3', () => {}); + test('test4', () => {}); + `, + }, { workers: 1 }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(4); + expect(countTimes(stripAnsi(result.output), '%%beforeAll')).toBe(1); + expect(countTimes(stripAnsi(result.output), '%%afterAll')).toBe(1); +}); + +test('parallel mode should minimize running beforeAll/afterAll hooks 2', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'a.test.ts': ` + const { test } = pwt; + test.describe.configure({ mode: 'parallel' }); + test.beforeAll(() => { + console.log('\\n%%beforeAll'); + }); + test.afterAll(() => { + console.log('\\n%%afterAll'); + }); + test('test1', () => {}); + test('test2', () => {}); + test('test3', () => {}); + test('test4', () => {}); + `, + }, { workers: 2 }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(4); + expect(countTimes(stripAnsi(result.output), '%%beforeAll')).toBe(2); + expect(countTimes(stripAnsi(result.output), '%%afterAll')).toBe(2); +});