From d33e8daeedbe6bb251412691a65ebdcc602ed791 Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Wed, 29 May 2024 11:23:59 +0200 Subject: [PATCH 1/3] fix(merge-reports) only change test ids when needed --- packages/playwright/src/reporters/merge.ts | 42 ++++++++++-- tests/playwright-test/reporter-blob.spec.ts | 72 +++++++++++++++++++++ 2 files changed, 110 insertions(+), 4 deletions(-) diff --git a/packages/playwright/src/reporters/merge.ts b/packages/playwright/src/reporters/merge.ts index 17272a90d8..e91c7305ec 100644 --- a/packages/playwright/src/reporters/merge.ts +++ b/packages/playwright/src/reporters/merge.ts @@ -194,16 +194,25 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool: printStatus(`merging events`); const reports: ReportData[] = []; + const testIdsUsedInOtherBlobs = new Set(); for (let i = 0; i < blobs.length; ++i) { // Generate unique salt for each blob. const { parsedEvents, metadata, localPath } = blobs[i]; const eventPatchers = new JsonEventPatchers(); - eventPatchers.patchers.push(new IdsPatcher(stringPool, metadata.name, String(i))); + const testIdsUsedInThisBlob = new Set(); + eventPatchers.patchers.push(new IdsPatcher( + stringPool, + metadata.name, + String(i), + testIdsUsedInThisBlob, + testIdsUsedInOtherBlobs, + )); // Only patch path separators if we are merging reports with explicit config. if (rootDirOverride) eventPatchers.patchers.push(new PathSeparatorPatcher(metadata.pathSeparator)); eventPatchers.patchEvents(parsedEvents); + testIdsUsedInThisBlob.forEach(id => testIdsUsedInOtherBlobs.add(id)); for (const event of parsedEvents) { if (event.method === 'onConfigure') @@ -358,11 +367,23 @@ class IdsPatcher { private _stringPool: StringInternPool; private _botName: string | undefined; private _salt: string; + private _testIdsMap: Map; + private _testIdsUsedInThisBlob: Set; + private _testIdsUsedInOtherBlobs: Set; - constructor(stringPool: StringInternPool, botName: string | undefined, salt: string) { + constructor( + stringPool: StringInternPool, + botName: string | undefined, + salt: string, + testIdsUsedInThisBlob: Set, + testIdsUsedInOtherBlobs: Set, + ) { this._stringPool = stringPool; this._botName = botName; this._salt = salt; + this._testIdsMap = new Map(); + this._testIdsUsedInThisBlob = testIdsUsedInThisBlob; + this._testIdsUsedInOtherBlobs = testIdsUsedInOtherBlobs; } patchEvent(event: JsonEvent) { @@ -406,7 +427,20 @@ class IdsPatcher { } private _mapTestId(testId: string): string { - return this._stringPool.internString(testId + this._salt); + const t1 = this._stringPool.internString(testId); + if (this._testIdsMap.has(t1)) + // already mapped + return this._testIdsMap.get(t1)!; + if (this._testIdsUsedInOtherBlobs.has(t1)) { + // test id is used in another blob, so we need to salt it. + const t2 = this._stringPool.internString(testId + this._salt); + this._testIdsUsedInThisBlob.add(t2); + this._testIdsMap.set(t1, t2); + return t2; + } + this._testIdsUsedInThisBlob.add(t1); + this._testIdsMap.set(t1, t1); + return t1; } } @@ -551,4 +585,4 @@ class BlobModernizer { } } -const modernizer = new BlobModernizer(); \ No newline at end of file +const modernizer = new BlobModernizer(); diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index ca3020a43f..29928a01f5 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -1813,6 +1813,78 @@ test('merge reports without --config preserves path separators', async ({ runInl expect(output).toContain(`test title: ${'tests2' + otherSeparator + 'b.test.js'}`); }); +test('merge reports must not change test ids when there is no need to', async ({ runInlineTest, mergeReports }) => { + const files = { + 'echo-test-id-reporter.js': ` + export default class EchoTestIdReporter { + onTestBegin(test) { + console.log('test.id:', test.id); + } + }; + `, + 'single-run.config.ts': `module.exports = { + reporter: [ + ['./echo-test-id-reporter.js'], + ] + };`, + 'shard-1.config.ts': `module.exports = { + fullyParallel: true, + shard: { total: 2, current: 1 }, + reporter: [ + ['./echo-test-id-reporter.js'], + ['blob', { outputDir: 'blob-report' }], + ] + };`, + 'shard-2.config.ts': `module.exports = { + fullyParallel: true, + shard: { total: 2, current: 2 }, + reporter: [ + ['./echo-test-id-reporter.js'], + ['blob', { outputDir: 'blob-report' }], + ] + };`, + 'merge.config.ts': `module.exports = { + reporter: [ + ['./echo-test-id-reporter.js'], + ] + };`, + 'a.test.js': ` + import { test, expect } from '@playwright/test'; + test('test 1', async ({}) => { }); + test('test 2', async ({}) => { }); + test('test 3', async ({}) => { }); + `, + }; + let testIdsFromSingleRun: string[]; + let testIdsFromShard1: string[]; + let testIdsFromShard2: string[]; + let testIdsFromMergedReport: string[]; + const parseTestIdsFromOutput = (output: string) => output.split('\n').filter(s => s.startsWith('test.id:')).map(s => s.split('test.id:')[1].trim()).sort(); + { + const { exitCode, output } = await runInlineTest(files, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('single-run.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromSingleRun = parseTestIdsFromOutput(output); + expect(testIdsFromSingleRun.length).toEqual(3); + } + { + const { exitCode, output } = await runInlineTest(files, { workers: 1 }, {}, { additionalArgs: ['--config', test.info().outputPath('shard-1.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromShard1 = parseTestIdsFromOutput(output); + } + { + const { exitCode, output } = await runInlineTest(files, { workers: 1 }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' }, { additionalArgs: ['--config', test.info().outputPath('shard-2.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromShard2 = parseTestIdsFromOutput(output); + expect([...testIdsFromShard1, ...testIdsFromShard2].sort()).toEqual(testIdsFromSingleRun); + } + { + const { exitCode, output } = await mergeReports(test.info().outputPath('blob-report'), undefined, { additionalArgs: ['--config', test.info().outputPath('merge.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromMergedReport = parseTestIdsFromOutput(output); + expect(testIdsFromMergedReport).toEqual(testIdsFromSingleRun); + } +}); + test('TestSuite.project() should return owning project', async ({ runInlineTest, mergeReports }) => { test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/29173' }); const files1 = { From 32169b7950fda91fcf65f0a44049d6ffba400e07 Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Thu, 30 May 2024 08:14:54 +0200 Subject: [PATCH 2/3] optimize: use single global set --- packages/playwright/src/reporters/merge.ts | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/packages/playwright/src/reporters/merge.ts b/packages/playwright/src/reporters/merge.ts index e91c7305ec..5eedcac136 100644 --- a/packages/playwright/src/reporters/merge.ts +++ b/packages/playwright/src/reporters/merge.ts @@ -194,25 +194,22 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool: printStatus(`merging events`); const reports: ReportData[] = []; - const testIdsUsedInOtherBlobs = new Set(); + const globalTestIdSet = new Set(); for (let i = 0; i < blobs.length; ++i) { // Generate unique salt for each blob. const { parsedEvents, metadata, localPath } = blobs[i]; const eventPatchers = new JsonEventPatchers(); - const testIdsUsedInThisBlob = new Set(); eventPatchers.patchers.push(new IdsPatcher( stringPool, metadata.name, String(i), - testIdsUsedInThisBlob, - testIdsUsedInOtherBlobs, + globalTestIdSet, )); // Only patch path separators if we are merging reports with explicit config. if (rootDirOverride) eventPatchers.patchers.push(new PathSeparatorPatcher(metadata.pathSeparator)); eventPatchers.patchEvents(parsedEvents); - testIdsUsedInThisBlob.forEach(id => testIdsUsedInOtherBlobs.add(id)); for (const event of parsedEvents) { if (event.method === 'onConfigure') @@ -368,22 +365,19 @@ class IdsPatcher { private _botName: string | undefined; private _salt: string; private _testIdsMap: Map; - private _testIdsUsedInThisBlob: Set; - private _testIdsUsedInOtherBlobs: Set; + private _globalTestIdSet: Set; constructor( stringPool: StringInternPool, botName: string | undefined, salt: string, - testIdsUsedInThisBlob: Set, - testIdsUsedInOtherBlobs: Set, + globalTestIdSet: Set, ) { this._stringPool = stringPool; this._botName = botName; this._salt = salt; this._testIdsMap = new Map(); - this._testIdsUsedInThisBlob = testIdsUsedInThisBlob; - this._testIdsUsedInOtherBlobs = testIdsUsedInOtherBlobs; + this._globalTestIdSet = globalTestIdSet; } patchEvent(event: JsonEvent) { @@ -431,14 +425,14 @@ class IdsPatcher { if (this._testIdsMap.has(t1)) // already mapped return this._testIdsMap.get(t1)!; - if (this._testIdsUsedInOtherBlobs.has(t1)) { + if (this._globalTestIdSet.has(t1)) { // test id is used in another blob, so we need to salt it. const t2 = this._stringPool.internString(testId + this._salt); - this._testIdsUsedInThisBlob.add(t2); + this._globalTestIdSet.add(t2); this._testIdsMap.set(t1, t2); return t2; } - this._testIdsUsedInThisBlob.add(t1); + this._globalTestIdSet.add(t1); this._testIdsMap.set(t1, t1); return t1; } From fd904241712353dc6767e3a4abf0f4fbb234c62b Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Thu, 30 May 2024 08:48:54 +0200 Subject: [PATCH 3/3] Feedback use outputLines --- .../playwright-test-fixtures.ts | 14 +++++- tests/playwright-test/reporter-blob.spec.ts | 43 +++++++++---------- 2 files changed, 33 insertions(+), 24 deletions(-) diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index 62e20d6945..4f0350cb59 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -31,6 +31,7 @@ export { countTimes } from '../config/commonFixtures'; type CliRunResult = { exitCode: number, output: string, + outputLines: string[], }; export type RunResult = { @@ -330,7 +331,12 @@ export const test = base cwd, }); const { exitCode } = await testProcess.exited; - return { exitCode, output: testProcess.output.toString() }; + const output = testProcess.output.toString(); + return { + exitCode, + output, + outputLines: parseOutputLines(output), + }; }); }, @@ -416,6 +422,10 @@ export function expectTestHelper(result: RunResult) { }; } +function parseOutputLines(output: string): string[] { + return output.split('\n').filter(line => line.startsWith('%%')).map(line => line.substring(2).trim()); +} + export function parseTestRunnerOutput(output: string) { const summary = (re: RegExp) => { let result = 0; @@ -436,7 +446,7 @@ export function parseTestRunnerOutput(output: string) { const strippedOutput = stripAnsi(output); return { output: strippedOutput, - outputLines: strippedOutput.split('\n').filter(line => line.startsWith('%%')).map(line => line.substring(2).trim()), + outputLines: parseOutputLines(strippedOutput), rawOutput: output, passed, failed, diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index 29928a01f5..4cc9b41886 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -32,18 +32,18 @@ const NEGATIVE_STATUS_MARK = DOES_NOT_SUPPORT_UTF8_IN_TERMINAL ? 'x ' : '✘ '; const test = baseTest.extend<{ showReport: (reportFolder?: string) => Promise - }>({ - showReport: async ({ page }, use) => { - let server: HttpServer | undefined; - await use(async (reportFolder?: string) => { - reportFolder ??= test.info().outputPath('playwright-report'); - server = startHtmlReportServer(reportFolder) as HttpServer; - await server.start(); - await page.goto(server.urlPrefix('precise')); - }); - await server?.stop(); - } - }); +}>({ + showReport: async ({ page }, use) => { + let server: HttpServer | undefined; + await use(async (reportFolder?: string) => { + reportFolder ??= test.info().outputPath('playwright-report'); + server = startHtmlReportServer(reportFolder) as HttpServer; + await server.start(); + await page.goto(server.urlPrefix('precise')); + }); + await server?.stop(); + } +}); test.use({ channel: 'chrome' }); test.slow(!!process.env.CI); @@ -1818,7 +1818,7 @@ test('merge reports must not change test ids when there is no need to', async ({ 'echo-test-id-reporter.js': ` export default class EchoTestIdReporter { onTestBegin(test) { - console.log('test.id:', test.id); + console.log('%%' + test.id); } }; `, @@ -1859,28 +1859,27 @@ test('merge reports must not change test ids when there is no need to', async ({ let testIdsFromShard1: string[]; let testIdsFromShard2: string[]; let testIdsFromMergedReport: string[]; - const parseTestIdsFromOutput = (output: string) => output.split('\n').filter(s => s.startsWith('test.id:')).map(s => s.split('test.id:')[1].trim()).sort(); { - const { exitCode, output } = await runInlineTest(files, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('single-run.config.ts')] }); + const { exitCode, outputLines } = await runInlineTest(files, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('single-run.config.ts')] }); expect(exitCode).toBe(0); - testIdsFromSingleRun = parseTestIdsFromOutput(output); + testIdsFromSingleRun = outputLines.sort(); expect(testIdsFromSingleRun.length).toEqual(3); } { - const { exitCode, output } = await runInlineTest(files, { workers: 1 }, {}, { additionalArgs: ['--config', test.info().outputPath('shard-1.config.ts')] }); + const { exitCode, outputLines } = await runInlineTest(files, { workers: 1 }, {}, { additionalArgs: ['--config', test.info().outputPath('shard-1.config.ts')] }); expect(exitCode).toBe(0); - testIdsFromShard1 = parseTestIdsFromOutput(output); + testIdsFromShard1 = outputLines.sort(); } { - const { exitCode, output } = await runInlineTest(files, { workers: 1 }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' }, { additionalArgs: ['--config', test.info().outputPath('shard-2.config.ts')] }); + const { exitCode, outputLines } = await runInlineTest(files, { workers: 1 }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' }, { additionalArgs: ['--config', test.info().outputPath('shard-2.config.ts')] }); expect(exitCode).toBe(0); - testIdsFromShard2 = parseTestIdsFromOutput(output); + testIdsFromShard2 = outputLines.sort(); expect([...testIdsFromShard1, ...testIdsFromShard2].sort()).toEqual(testIdsFromSingleRun); } { - const { exitCode, output } = await mergeReports(test.info().outputPath('blob-report'), undefined, { additionalArgs: ['--config', test.info().outputPath('merge.config.ts')] }); + const { exitCode, outputLines } = await mergeReports(test.info().outputPath('blob-report'), undefined, { additionalArgs: ['--config', test.info().outputPath('merge.config.ts')] }); expect(exitCode).toBe(0); - testIdsFromMergedReport = parseTestIdsFromOutput(output); + testIdsFromMergedReport = outputLines.sort(); expect(testIdsFromMergedReport).toEqual(testIdsFromSingleRun); } });