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 = {