From d33e8daeedbe6bb251412691a65ebdcc602ed791 Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Wed, 29 May 2024 11:23:59 +0200 Subject: [PATCH] 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 = {