From 570814849657da8fe86a90269e239f41e718750d Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Thu, 30 May 2024 20:29:20 +0200 Subject: [PATCH] fix(merge-reports): only change test ids when needed (#31061) When merging blob reports test ids are patched to make sure there is no collision when merging reports that might have overlapping test ids. However, even if you were merging reports that had no overlapping ids, all test ids will be modified, which is an undesirable side effect. This PR only modify test ids when the same test id has already been used in a previous blob report. ---- This change is also part of https://github.com/microsoft/playwright/pull/30962 --- packages/playwright/src/reporters/merge.ts | 36 ++++++++- .../playwright-test-fixtures.ts | 14 +++- tests/playwright-test/reporter-blob.spec.ts | 73 ++++++++++++++++++- 3 files changed, 116 insertions(+), 7 deletions(-) diff --git a/packages/playwright/src/reporters/merge.ts b/packages/playwright/src/reporters/merge.ts index 17272a90d8..5eedcac136 100644 --- a/packages/playwright/src/reporters/merge.ts +++ b/packages/playwright/src/reporters/merge.ts @@ -194,12 +194,18 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool: printStatus(`merging events`); const reports: ReportData[] = []; + 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(); - eventPatchers.patchers.push(new IdsPatcher(stringPool, metadata.name, String(i))); + eventPatchers.patchers.push(new IdsPatcher( + stringPool, + metadata.name, + String(i), + globalTestIdSet, + )); // Only patch path separators if we are merging reports with explicit config. if (rootDirOverride) eventPatchers.patchers.push(new PathSeparatorPatcher(metadata.pathSeparator)); @@ -358,11 +364,20 @@ class IdsPatcher { private _stringPool: StringInternPool; private _botName: string | undefined; private _salt: string; + private _testIdsMap: Map; + private _globalTestIdSet: Set; - constructor(stringPool: StringInternPool, botName: string | undefined, salt: string) { + constructor( + stringPool: StringInternPool, + botName: string | undefined, + salt: string, + globalTestIdSet: Set, + ) { this._stringPool = stringPool; this._botName = botName; this._salt = salt; + this._testIdsMap = new Map(); + this._globalTestIdSet = globalTestIdSet; } patchEvent(event: JsonEvent) { @@ -406,7 +421,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._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._globalTestIdSet.add(t2); + this._testIdsMap.set(t1, t2); + return t2; + } + this._globalTestIdSet.add(t1); + this._testIdsMap.set(t1, t1); + return t1; } } @@ -551,4 +579,4 @@ class BlobModernizer { } } -const modernizer = new BlobModernizer(); \ No newline at end of file +const modernizer = new BlobModernizer(); 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 ca3020a43f..99882777a8 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -36,7 +36,7 @@ const test = baseTest.extend<{ showReport: async ({ page }, use) => { let server: HttpServer | undefined; await use(async (reportFolder?: string) => { - reportFolder ??= test.info().outputPath('playwright-report'); + reportFolder ??= test.info().outputPath('playwright-report'); server = startHtmlReportServer(reportFolder) as HttpServer; await server.start(); await page.goto(server.urlPrefix('precise')); @@ -1813,6 +1813,77 @@ 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); + } + }; + `, + '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 { exitCode, outputLines } = await runInlineTest(files, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('single-run.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromSingleRun = outputLines.sort(); + expect(testIdsFromSingleRun.length).toEqual(3); + } + { + const { exitCode, outputLines } = await runInlineTest(files, { workers: 1 }, {}, { additionalArgs: ['--config', test.info().outputPath('shard-1.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromShard1 = outputLines.sort(); + } + { + 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 = outputLines.sort(); + expect([...testIdsFromShard1, ...testIdsFromShard2].sort()).toEqual(testIdsFromSingleRun); + } + { + const { exitCode, outputLines } = await mergeReports(test.info().outputPath('blob-report'), undefined, { additionalArgs: ['--config', test.info().outputPath('merge.config.ts')] }); + expect(exitCode).toBe(0); + testIdsFromMergedReport = outputLines.sort(); + 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 = {