Skip to content

Commit

Permalink
cherry-pick(#13138): fix(parallel): minimize the number of beforeAll/…
Browse files Browse the repository at this point in the history
…afterAll hooks in parallel mode (#13140)

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.
  • Loading branch information
dgozman authored Mar 29, 2022
1 parent 18de7bd commit e55a3ff
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 8 deletions.
40 changes: 33 additions & 7 deletions packages/playwright-test/src/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,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) {
Expand Down Expand Up @@ -619,7 +619,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.
Expand All @@ -630,7 +630,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<string, Map<string, { general: TestGroup, parallel: TestGroup[] }>>();
const groups = new Map<string, Map<string, {
// Tests that must be run in order are in the same group.
general: TestGroup,
// Tests that may be run independently each has a dedicated group with a single test.
parallel: TestGroup[],
// Tests that are marked as parallel but have beforeAll/afterAll hooks should be grouped
// as much as possible. We split them into equally sized groups, one per worker.
parallelWithHooks: TestGroup,
}>>();

const createGroup = (test: TestCase): TestGroup => {
return {
Expand All @@ -654,18 +662,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.length > 0;
}

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);
}
Expand All @@ -678,6 +694,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;
Expand Down
48 changes: 47 additions & 1 deletion tests/playwright-test/test-parallel.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -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);
});

0 comments on commit e55a3ff

Please sign in to comment.