From c7b58d76e4445d17a53fc5d1bc2a5221003e4815 Mon Sep 17 00:00:00 2001 From: Mathias Leppich Date: Thu, 30 May 2024 08:51:16 +0200 Subject: [PATCH] Revert "fix(merge-reports) only change test ids when needed" This reverts commit 389e5711f46f7bf6b0be98ca15795e17e4d3958b. --- packages/playwright/src/reporters/merge.ts | 42 ++---------- tests/playwright-test/reporter-blob.spec.ts | 72 --------------------- 2 files changed, 4 insertions(+), 110 deletions(-) diff --git a/packages/playwright/src/reporters/merge.ts b/packages/playwright/src/reporters/merge.ts index e91c7305ec..17272a90d8 100644 --- a/packages/playwright/src/reporters/merge.ts +++ b/packages/playwright/src/reporters/merge.ts @@ -194,25 +194,16 @@ 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(); - const testIdsUsedInThisBlob = new Set(); - eventPatchers.patchers.push(new IdsPatcher( - stringPool, - metadata.name, - String(i), - testIdsUsedInThisBlob, - testIdsUsedInOtherBlobs, - )); + eventPatchers.patchers.push(new IdsPatcher(stringPool, metadata.name, String(i))); // 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') @@ -367,23 +358,11 @@ 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, - testIdsUsedInThisBlob: Set, - testIdsUsedInOtherBlobs: Set, - ) { + constructor(stringPool: StringInternPool, botName: string | undefined, salt: string) { this._stringPool = stringPool; this._botName = botName; this._salt = salt; - this._testIdsMap = new Map(); - this._testIdsUsedInThisBlob = testIdsUsedInThisBlob; - this._testIdsUsedInOtherBlobs = testIdsUsedInOtherBlobs; } patchEvent(event: JsonEvent) { @@ -427,20 +406,7 @@ class IdsPatcher { } private _mapTestId(testId: string): string { - 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; + return this._stringPool.internString(testId + this._salt); } } @@ -585,4 +551,4 @@ class BlobModernizer { } } -const modernizer = new BlobModernizer(); +const modernizer = new BlobModernizer(); \ No newline at end of file diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index 29928a01f5..ca3020a43f 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -1813,78 +1813,6 @@ 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 = {