Merge branch 'fix-merge-reports-test-ids' into sharding-algorithm

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

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,

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

@@ -32,18 +32,18 @@ const NEGATIVE_STATUS_MARK = DOES_NOT_SUPPORT_UTF8_IN_TERMINAL ? 'x ' : '✘ ';
 
 const test = baseTest.extend<{
   showReport: (reportFolder?: string) => Promise<void>
-      }>({
-        showReport: async ({ page }, use) => {
-          let server: HttpServer | undefined;
-          await use(async (reportFolder?: string) => {
-            reportFolder ??=  test.info().outputPath('playwright-report');
-            server = startHtmlReportServer(reportFolder) as HttpServer;
-            await server.start();
-            await page.goto(server.urlPrefix('precise'));
-          });
-          await server?.stop();
-        }
-      });
+}>({
+  showReport: async ({ page }, use) => {
+    let server: HttpServer | undefined;
+    await use(async (reportFolder?: string) => {
+      reportFolder ??= test.info().outputPath('playwright-report');
+      server = startHtmlReportServer(reportFolder) as HttpServer;
+      await server.start();
+      await page.goto(server.urlPrefix('precise'));
+    });
+    await server?.stop();
+  }
+});
 
 test.use({ channel: 'chrome' });
 test.slow(!!process.env.CI);
@@ -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 = {