fix(merge): normalize path separators when merging across platforms (#28227)

2025-06-26 21:40:17 +00:00 · 2023-11-27 12:43:56 -08:00 · 2023-11-27 12:43:56 -08:00 · dc8ecc3ca4
commit dc8ecc3ca4
parent 854ae4eede
4 changed files with 294 additions and 67 deletions
--- a/packages/playwright/src/reporters/blob.ts
+++ b/packages/playwright/src/reporters/blob.ts
@ -38,6 +38,7 @@ export type BlobReportMetadata = {
  userAgent: string;
  name?: string;
  shard?: { total: number, current: number };
+  pathSeparator?: string;
 };

 export class BlobReporter extends TeleReporterEmitter {
@ -59,6 +60,7 @@ export class BlobReporter extends TeleReporterEmitter {
      userAgent: getUserAgent(),
      name: process.env.PWTEST_BLOB_REPORT_NAME,
      shard: config.shard ?? undefined,
+      pathSeparator: path.sep,
    };
    this._messages.push({
      method: 'onBlobReportMetadata',
--- a/packages/playwright/src/reporters/merge.ts
+++ b/packages/playwright/src/reporters/merge.ts
@ -18,7 +18,7 @@ import fs from 'fs';
 import path from 'path';
 import type { ReporterDescription } from '../../types/test';
 import type { FullConfigInternal } from '../common/config';
-import type { JsonConfig, JsonEvent, JsonFullResult, JsonProject, JsonSuite, JsonTestResultEnd } from '../isomorphic/teleReceiver';
+import type { JsonConfig, JsonEvent, JsonFullResult, JsonLocation, JsonProject, JsonSuite, JsonTestResultEnd, JsonTestStepStart } from '../isomorphic/teleReceiver';
 import { TeleReporterReceiver } from '../isomorphic/teleReceiver';
 import { JsonStringInternalizer, StringInternPool } from '../isomorphic/stringInternPool';
 import { createReporters } from '../runner/reporters';
@ -30,14 +30,13 @@ import { relativeFilePath } from '../util';
 type StatusCallback = (message: string) => void;

 type ReportData = {
-  idsPatcher: IdsPatcher;
+  eventPatchers: JsonEventPatchers;
  reportFile: string;
 };

 export async function createMergedReport(config: FullConfigInternal, dir: string, reporterDescriptions: ReporterDescription[], rootDirOverride: string | undefined) {
  const reporters = await createReporters(config, 'merge', reporterDescriptions);
  const multiplexer = new Multiplexer(reporters);
-  const receiver = new TeleReporterReceiver(path.sep, multiplexer, false, config.config);
  const stringPool = new StringInternPool();

  let printStatus: StatusCallback = () => {};
@ -50,6 +49,9 @@ export async function createMergedReport(config: FullConfigInternal, dir: string
  if (shardFiles.length === 0)
    throw new Error(`No report files found in ${dir}`);
  const eventData = await mergeEvents(dir, shardFiles, stringPool, printStatus, rootDirOverride);
+  // If expicit config is provided, use platform path separator, otherwise use the one from the report (if any).
+  const pathSep = rootDirOverride ? path.sep : (eventData.pathSeparatorFromMetadata ?? path.sep);
+  const receiver = new TeleReporterReceiver(pathSep, multiplexer, false, config.config);
  printStatus(`processing test events`);

  const dispatchEvents = async (events: JsonEvent[]) => {
@ -63,30 +65,17 @@ export async function createMergedReport(config: FullConfigInternal, dir: string
  };

  await dispatchEvents(eventData.prologue);
-  for (const { reportFile, idsPatcher } of eventData.reports) {
+  for (const { reportFile, eventPatchers } of eventData.reports) {
    const reportJsonl = await fs.promises.readFile(reportFile);
    const events = parseTestEvents(reportJsonl);
    new JsonStringInternalizer(stringPool).traverse(events);
-    idsPatcher.patchEvents(events);
-    patchAttachmentPaths(events, dir);
+    eventPatchers.patchers.push(new AttachmentPathPatcher(dir));
+    eventPatchers.patchEvents(events);
    await dispatchEvents(events);
  }
  await dispatchEvents(eventData.epilogue);
 }

-function patchAttachmentPaths(events: JsonEvent[], resourceDir: string) {
-  for (const event of events) {
-    if (event.method !== 'onTestEnd')
-      continue;
-    for (const attachment of (event.params.result as JsonTestResultEnd).attachments) {
-      if (!attachment.path)
-        continue;
-
-      attachment.path = path.join(resourceDir, attachment.path);
-    }
-  }
-}
-
 const commonEventNames = ['onBlobReportMetadata', 'onConfigure', 'onProject', 'onBegin', 'onEnd'];
 const commonEvents = new Set(commonEventNames);
 const commonEventRegex = new RegExp(`${commonEventNames.join('|')}`);
@ -186,8 +175,12 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool:
      salt = sha1 + '-' + i;
    saltSet.add(salt);

-    const idsPatcher = new IdsPatcher(stringPool, metadata.name, salt);
-    idsPatcher.patchEvents(parsedEvents);
+    const eventPatchers = new JsonEventPatchers();
+    eventPatchers.patchers.push(new IdsPatcher(stringPool, metadata.name, salt));
+    // Only patch path separators if we are merging reports with explicit config.
+    if (rootDirOverride)
+      eventPatchers.patchers.push(new PathSeparatorPatcher(metadata.pathSeparator));
+    eventPatchers.patchEvents(parsedEvents);

    for (const event of parsedEvents) {
      if (event.method === 'onConfigure')
@ -200,7 +193,7 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool:

    // Save information about the reports to stream their test events later.
    reports.push({
-      idsPatcher,
+      eventPatchers,
      reportFile: localPath,
    });
  }
@ -215,7 +208,8 @@ async function mergeEvents(dir: string, shardReportFiles: string[], stringPool:
    epilogue: [
      mergeEndEvents(endEvents),
      { method: 'onExit', params: undefined },
-    ]
+    ],
+    pathSeparatorFromMetadata: blobs[0]?.metadata.pathSeparator,
  };
 }

@ -338,26 +332,27 @@ class UniqueFileNameGenerator {
 }

 class IdsPatcher {
-  constructor(private _stringPool: StringInternPool, private _reportName: string | undefined, private _salt: string) {
+  constructor(
+    private _stringPool: StringInternPool,
+    private _reportName: string | undefined,
+    private _salt: string) {
  }

-  patchEvents(events: JsonEvent[]) {
-    for (const event of events) {
-      const { method, params } = event;
-      switch (method) {
-        case 'onProject':
-          this._onProject(params.project);
-          continue;
-        case 'onTestBegin':
-        case 'onStepBegin':
-        case 'onStepEnd':
-        case 'onStdIO':
-          params.testId = this._mapTestId(params.testId);
-          continue;
-        case 'onTestEnd':
-          params.test.testId = this._mapTestId(params.test.testId);
-          continue;
-      }
+  patchEvent(event: JsonEvent) {
+    const { method, params } = event;
+    switch (method) {
+      case 'onProject':
+        this._onProject(params.project);
+        return;
+      case 'onTestBegin':
+      case 'onStepBegin':
+      case 'onStepEnd':
+      case 'onStdIO':
+        params.testId = this._mapTestId(params.testId);
+        return;
+      case 'onTestEnd':
+        params.test.testId = this._mapTestId(params.test.testId);
+        return;
    }
  }

@ -377,3 +372,92 @@ class IdsPatcher {
    return this._stringPool.internString(testId + this._salt);
  }
 }
+
+class AttachmentPathPatcher {
+  constructor(private _resourceDir: string) {
+  }
+
+  patchEvent(event: JsonEvent) {
+    if (event.method !== 'onTestEnd')
+      return;
+    for (const attachment of (event.params.result as JsonTestResultEnd).attachments) {
+      if (!attachment.path)
+        continue;
+
+      attachment.path = path.join(this._resourceDir, attachment.path);
+    }
+  }
+}
+
+class PathSeparatorPatcher {
+  private _from: string;
+  private _to: string;
+  constructor(from?: string) {
+    this._from = from ?? (path.sep === '/' ? '\\' : '/');
+    this._to = path.sep;
+  }
+
+  patchEvent(jsonEvent: JsonEvent) {
+    if (this._from === this._to)
+      return;
+    if (jsonEvent.method === 'onProject') {
+      this._updateProject(jsonEvent.params.project as JsonProject);
+      return;
+    }
+    if (jsonEvent.method === 'onTestEnd') {
+      const testResult = jsonEvent.params.result as JsonTestResultEnd;
+      testResult.errors.forEach(error => this._updateLocation(error.location));
+      testResult.attachments.forEach(attachment => {
+        if (attachment.path)
+          attachment.path = this._updatePath(attachment.path);
+      });
+      return;
+    }
+    if (jsonEvent.method === 'onStepBegin') {
+      const step = jsonEvent.params.step as JsonTestStepStart;
+      this._updateLocation(step.location);
+      return;
+    }
+  }
+
+  private _updateProject(project: JsonProject) {
+    project.outputDir = this._updatePath(project.outputDir);
+    project.testDir = this._updatePath(project.testDir);
+    project.snapshotDir = this._updatePath(project.snapshotDir);
+    project.suites.forEach(suite => this._updateSuite(suite, true));
+  }
+
+  private _updateSuite(suite: JsonSuite, isFileSuite: boolean = false) {
+    this._updateLocation(suite.location);
+    if (isFileSuite)
+      suite.title = this._updatePath(suite.title);
+    for (const child of suite.suites)
+      this._updateSuite(child);
+    for (const test of suite.tests)
+      this._updateLocation(test.location);
+  }
+
+  private _updateLocation(location?: JsonLocation) {
+    if (location)
+      location.file = this._updatePath(location.file);
+  }
+
+  private _updatePath(text: string): string {
+    return text.split(this._from).join(this._to);
+  }
+}
+
+interface JsonEventPatcher {
+  patchEvent(event: JsonEvent): void;
+}
+
+class JsonEventPatchers {
+  readonly patchers: JsonEventPatcher[] = [];
+
+  patchEvents(events: JsonEvent[]) {
+    for (const event of events) {
+      for (const patcher of this.patchers)
+        patcher.patchEvent(event);
+    }
+  }
+}
--- a/tests/playwright-test/reporter-blob.spec.ts
+++ b/tests/playwright-test/reporter-blob.spec.ts
@ -1259,15 +1259,36 @@ test('blob report should include version', async ({ runInlineTest }) => {
  const reportFiles = await fs.promises.readdir(reportDir);
  expect(reportFiles).toEqual(['report.zip']);

-  await extractZip(test.info().outputPath('blob-report', reportFiles[0]), { dir: test.info().outputPath('blob-report') });
-  const reportFile = test.info().outputPath('blob-report', reportFiles[0].replace(/\.zip$/, '.jsonl'));
-  const data = await fs.promises.readFile(reportFile, 'utf8');
-  const events = data.split('\n').filter(Boolean).map(line => JSON.parse(line));
+  const events = await extractReport(test.info().outputPath('blob-report', 'report.zip'), test.info().outputPath('tmp'));
  const metadataEvent = events.find(e => e.method === 'onBlobReportMetadata');
  expect(metadataEvent.params.version).toBe(1);
  expect(metadataEvent.params.userAgent).toBe(getUserAgent());
 });

+async function extractReport(reportZipFile: string, unzippedReportDir: string): Promise<any[]> {
+  await extractZip(reportZipFile, { dir: unzippedReportDir });
+  const reportFile = path.join(unzippedReportDir, path.basename(reportZipFile).replace(/\.zip$/, '.jsonl'));
+  const data = await fs.promises.readFile(reportFile, 'utf8');
+  const events = data.split('\n').filter(Boolean).map(line => JSON.parse(line));
+  return events;
+}
+
+async function zipReport(events: any[], zipFilePath: string) {
+  const lines = events.map(e => JSON.stringify(e) + '\n');
+  const zipFile = new yazl.ZipFile();
+  const zipFinishPromise = new Promise((resolve, reject) => {
+    (zipFile as any).on('error', error => reject(error));
+    zipFile.outputStream.pipe(fs.createWriteStream(zipFilePath)).on('close', () => {
+      resolve(undefined);
+    }).on('error', error => reject(error));
+  });
+  const content = Readable.from(lines);
+  zipFile.addReadStream(content, 'report.jsonl');
+  zipFile.end();
+  await zipFinishPromise;
+}
+
+
 test('merge-reports should throw if report version is from the future', async ({ runInlineTest, mergeReports }) => {
  const reportDir = test.info().outputPath('blob-report');
  const files = {
@ -1293,28 +1314,15 @@ test('merge-reports should throw if report version is from the future', async ({

  // Extract report and modify version.
  const reportZipFile = test.info().outputPath('blob-report', reportFiles[1]);
-  await extractZip(reportZipFile, { dir: test.info().outputPath('tmp') });
-  const reportFile = test.info().outputPath('tmp', reportFiles[1].replace(/\.zip$/, '.jsonl'));
-  const data = await fs.promises.readFile(reportFile, 'utf8');
-  const events = data.split('\n').filter(Boolean).map(line => JSON.parse(line));
+  const events = await extractReport(reportZipFile, test.info().outputPath('tmp'));
+
  const metadataEvent = events.find(e => e.method === 'onBlobReportMetadata');
  expect(metadataEvent.params.version).toBeTruthy();
  ++metadataEvent.params.version;
-  const modifiedLines = events.map(e => JSON.stringify(e) + '\n');

  // Zip it back.
  await fs.promises.rm(reportZipFile, { force: true });
-  const zipFile = new yazl.ZipFile();
-  const zipFinishPromise = new Promise((resolve, reject) => {
-    (zipFile as any).on('error', error => reject(error));
-    zipFile.outputStream.pipe(fs.createWriteStream(reportZipFile)).on('close', () => {
-      resolve(undefined);
-    }).on('error', error => reject(error));
-  });
-  const content = Readable.from(modifiedLines);
-  zipFile.addReadStream(content, path.basename(reportFile));
-  zipFile.end();
-  await zipFinishPromise;
+  await zipReport(events, reportZipFile);

  const { exitCode, output } = await mergeReports(reportDir, { 'PW_TEST_HTML_REPORT_OPEN': 'never' }, { additionalArgs: ['--reporter', 'html'] });
  expect(exitCode).toBe(1);
@ -1512,3 +1520,135 @@ test('merge reports same rootDirs', async ({ runInlineTest, mergeReports }) => {
  expect(output).toContain(`test: ${test.info().outputPath('tests', 'dir1', 'a.test.js')}`);
  expect(output).toContain(`test: ${test.info().outputPath('tests', 'dir2', 'b.test.js')}`);
 });
+
+
+function patchPathSeparators(json: any) {
+  const from = (path.sep === '/') ? /\//g : /\\/g;
+  const to = (path.sep === '/') ? '\\' : '/';
+
+  function patchPathSeparatorsRecursive(obj: any) {
+    if (typeof obj !== 'object')
+      return;
+    for (const key in obj) {
+      if (/file|dir|path|^title$/i.test(key) && typeof obj[key] === 'string')
+        obj[key] = obj[key].replace(from, to);
+      patchPathSeparatorsRecursive(obj[key]);
+    }
+  }
+  patchPathSeparatorsRecursive(json);
+}
+
+test('merge reports with different rootDirs and path separators', async ({ runInlineTest, mergeReports }) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/27877' });
+  const files1 = {
+    'echo-reporter.js': `
+      export default class EchoReporter {
+        onBegin(config, suite) {
+          console.log('rootDir:', config.rootDir);
+        }
+        onTestBegin(test) {
+          console.log('test:', test.location.file);
+          console.log('test title:', test.titlePath()[2]);
+        }
+      };
+    `,
+    'merge.config.ts': `module.exports = {
+      testDir: 'mergeRoot',
+      reporter: './echo-reporter.js'
+     };`,
+    'dir1/playwright.config.ts': `module.exports = {
+      reporter: [['blob', { outputDir: 'blob-report' }]]
+    };`,
+    'dir1/tests1/a.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('math 1', async ({}) => { });
+    `,
+  };
+  await runInlineTest(files1, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('dir1/playwright.config.ts')] });
+
+  const files2 = {
+    'dir2/playwright.config.ts': `module.exports = {
+      reporter: [['blob', { outputDir: 'blob-report' }]]
+    };`,
+    'dir2/tests2/b.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('math 2', async ({}) => { });
+    `,
+  };
+  await runInlineTest(files2, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('dir2/playwright.config.ts')] });
+
+  const allReportsDir = test.info().outputPath('all-blob-reports');
+  await fs.promises.mkdir(allReportsDir, { recursive: true });
+
+  // Extract report and change path separators.
+  const reportZipFile = test.info().outputPath('dir1', 'blob-report', 'report.zip');
+  const events = await extractReport(reportZipFile, test.info().outputPath('tmp'));
+  events.forEach(patchPathSeparators);
+
+  // Zip it back.
+  const report1 = path.join(allReportsDir, 'report-1.zip');
+  await zipReport(events, report1);
+
+  // Copy second report as is.
+  await fs.promises.cp(test.info().outputPath('dir2', 'blob-report', 'report.zip'), path.join(allReportsDir, 'report-2.zip'));
+
+  {
+    const { exitCode, output } = await mergeReports(allReportsDir, undefined, { additionalArgs: ['--config', 'merge.config.ts'] });
+    expect(exitCode).toBe(0);
+    expect(output).toContain(`rootDir: ${test.info().outputPath('mergeRoot')}`);
+    expect(output).toContain(`test: ${test.info().outputPath('mergeRoot', 'tests1', 'a.test.js')}`);
+    expect(output).toContain(`test title: ${'tests1' + path.sep + 'a.test.js'}`);
+    expect(output).toContain(`test: ${test.info().outputPath('mergeRoot', 'tests2', 'b.test.js')}`);
+    expect(output).toContain(`test title: ${'tests2' + path.sep + 'b.test.js'}`);
+  }
+});
+
+test('merge reports without --config preserves path separators', async ({ runInlineTest, mergeReports }) => {
+  test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/27877' });
+  const files1 = {
+    'echo-reporter.js': `
+      export default class EchoReporter {
+        onBegin(config, suite) {
+          console.log('rootDir:', config.rootDir);
+        }
+        onTestBegin(test) {
+          console.log('test:', test.location.file);
+          console.log('test title:', test.titlePath()[2]);
+        }
+      };
+    `,
+    'dir1/playwright.config.ts': `module.exports = {
+      reporter: [['blob', { outputDir: 'blob-report' }]]
+    };`,
+    'dir1/tests1/a.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('math 1', async ({}) => { });
+    `,
+    'dir1/tests2/b.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('math 2', async ({}) => { });
+    `,
+  };
+  await runInlineTest(files1, { workers: 1 }, undefined, { additionalArgs: ['--config', test.info().outputPath('dir1/playwright.config.ts')] });
+
+  const allReportsDir = test.info().outputPath('all-blob-reports');
+  await fs.promises.mkdir(allReportsDir, { recursive: true });
+
+  // Extract report and change path separators.
+  const reportZipFile = test.info().outputPath('dir1', 'blob-report', 'report.zip');
+  const events = await extractReport(reportZipFile, test.info().outputPath('tmp'));
+  events.forEach(patchPathSeparators);
+
+  // Zip it back.
+  const report1 = path.join(allReportsDir, 'report-1.zip');
+  await zipReport(events, report1);
+
+  const { exitCode, output } = await mergeReports(allReportsDir, undefined, { additionalArgs: ['--reporter', './echo-reporter.js'] });
+  expect(exitCode).toBe(0);
+  const otherSeparator = path.sep === '/' ? '\\' : '/';
+  expect(output).toContain(`rootDir: ${test.info().outputPath('dir1').replaceAll(path.sep, otherSeparator)}`);
+  expect(output).toContain(`test: ${test.info().outputPath('dir1', 'tests1', 'a.test.js').replaceAll(path.sep, otherSeparator)}`);
+  expect(output).toContain(`test title: ${'tests1' + otherSeparator + 'a.test.js'}`);
+  expect(output).toContain(`test: ${test.info().outputPath('dir1', 'tests2', 'b.test.js').replaceAll(path.sep, otherSeparator)}`);
+  expect(output).toContain(`test title: ${'tests2' + otherSeparator + 'b.test.js'}`);
+});
--- a/tests/playwright-test/reporter-markdown.spec.ts
+++ b/tests/playwright-test/reporter-markdown.spec.ts
@ -14,7 +14,8 @@
 * limitations under the License.
 */

-import * as fs from 'fs';
+import fs from 'fs';
+import path from 'path';
 import { expect, test } from './playwright-test-fixtures';

 test('simple report', async ({ runInlineTest }) => {
@ -25,7 +26,7 @@ test('simple report', async ({ runInlineTest }) => {
        reporter: 'markdown',
      };
    `,
-    'a.test.js': `
+    'dir1/a.test.js': `
      import { test, expect } from '@playwright/test';
      test('math 1', async ({}) => {
        expect(1 + 1).toBe(2);
@ -38,7 +39,7 @@ test('simple report', async ({ runInlineTest }) => {
      });
      test.skip('skipped 1', async ({}) => {});
    `,
-    'b.test.js': `
+    'dir2/b.test.js': `
      import { test, expect } from '@playwright/test';
      test('math 2', async ({}) => {
        expect(1 + 1).toBe(2);
@ -63,13 +64,13 @@ test('simple report', async ({ runInlineTest }) => {
  expect(exitCode).toBe(1);
  const reportFile = await fs.promises.readFile(test.info().outputPath('report.md'));
  expect(reportFile.toString()).toContain(`**2 failed**
-:x: a.test.js:6:11 › failing 1
-:x: b.test.js:6:11 › failing 2
+:x: dir1${path.sep}a.test.js:6:11 › failing 1
+:x: dir2${path.sep}b.test.js:6:11 › failing 2

 <details>
 <summary><b>2 flaky</b></summary>
-:warning: a.test.js:9:11 › flaky 1 <br/>
 :warning: c.test.js:6:11 › flaky 2 <br/>
+:warning: dir1${path.sep}a.test.js:9:11 › flaky 1 <br/>

 </details>