fix(merge-reports) only change test ids when needed

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<string>();
 
   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<string>();
+    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<string, string>;
+  private _testIdsUsedInThisBlob: Set<string>;
+  private _testIdsUsedInOtherBlobs: Set<string>;
 
-  constructor(stringPool: StringInternPool, botName: string | undefined, salt: string) {
+  constructor(
+    stringPool: StringInternPool,
+    botName: string | undefined,
+    salt: string,
+    testIdsUsedInThisBlob: Set<string>,
+    testIdsUsedInOtherBlobs: Set<string>,
+  ) {
     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();
+const modernizer = new BlobModernizer();

tests/playwright-test/reporter-blob.spec.ts

@@ -1781,6 +1781,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 = {