From 9c70a75d48b2f28a129cbe53eaadf2bdf16c2feb Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 27 Jul 2023 18:54:00 -0700 Subject: [PATCH] fix(merge): make sure testId from different blobs are unique (#24475) Fixes a scenario where each shard runs the same setup project. References #24451. --- .../playwright-test/src/reporters/merge.ts | 25 +++++++++++---- tests/playwright-test/reporter-blob.spec.ts | 32 +++++++++++++++---- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/packages/playwright-test/src/reporters/merge.ts b/packages/playwright-test/src/reporters/merge.ts index 900dfc7100..01d2fed9cd 100644 --- a/packages/playwright-test/src/reporters/merge.ts +++ b/packages/playwright-test/src/reporters/merge.ts @@ -96,6 +96,7 @@ async function mergeEvents(dir: string, shardReportFiles: string[]) { const shardB = b.metadata.shard?.current ?? 0; return shardA - shardB; }); + const allTestIds = new Set(); for (const { parsedEvents } of shardEvents) { for (const event of parsedEvents) { if (event.method === 'onConfigure') @@ -105,7 +106,7 @@ async function mergeEvents(dir: string, shardReportFiles: string[]) { else if (event.method === 'onEnd') endEvents.push(event); else if (event.method === 'onBlobReportMetadata') - new ProjectNamePatcher(event.params.projectSuffix).patchEvents(parsedEvents); + new ProjectNamePatcher(allTestIds, event.params.projectSuffix || '').patchEvents(parsedEvents); else events.push(event); } @@ -211,12 +212,12 @@ async function sortedShardFiles(dir: string) { } class ProjectNamePatcher { - constructor(private _projectNameSuffix: string) { + private _testIds = new Set(); + + constructor(private _allTestIds: Set, private _projectNameSuffix: string) { } patchEvents(events: JsonEvent[]) { - if (!this._projectNameSuffix) - return; for (const event of events) { const { method, params } = event; switch (method) { @@ -234,6 +235,8 @@ class ProjectNamePatcher { continue; } } + for (const testId of this._testIds) + this._allTestIds.add(testId); } private _onBegin(config: JsonConfig, projects: JsonProject[]) { @@ -259,11 +262,21 @@ class ProjectNamePatcher { } private _updateTestIds(suite: JsonSuite) { - suite.tests.forEach(test => test.testId = this._mapTestId(test.testId)); + suite.tests.forEach(test => { + test.testId = this._mapTestId(test.testId); + this._testIds.add(test.testId); + }); suite.suites.forEach(suite => this._updateTestIds(suite)); } private _mapTestId(testId: string): string { - return testId + '-' + this._projectNameSuffix; + testId = testId + this._projectNameSuffix; + // Consider a setup project running on each shard. In this case we'll have + // the same testId (from setup project) in multiple blob reports. + // To avoid reporters being confused by clashing test ids, we automatically + // make them unique and produce a separate test from each blob. + while (this._allTestIds.has(testId)) + testId = testId + '1'; + return testId; } } diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index bb2c33cede..8f0078bbd0 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -146,15 +146,25 @@ test('should call methods in right order', async ({ runInlineTest, mergeReports expect(lines.filter(l => l === 'onExit').length).toBe(1); }); -test('should merge into html', async ({ runInlineTest, mergeReports, showReport, page }) => { +test('should merge into html with dependencies', async ({ runInlineTest, mergeReports, showReport, page }) => { const reportDir = test.info().outputPath('blob-report'); const files = { 'playwright.config.ts': ` module.exports = { retries: 1, - reporter: [['blob', { outputDir: '${reportDir.replace(/\\/g, '/')}' }]] + reporter: [['blob', { outputDir: '${reportDir.replace(/\\/g, '/')}' }]], + projects: [ + { name: 'test', dependencies: ['setup'] }, + { name: 'setup', testMatch: /.*setup.js/ }, + ] }; `, + 'setup.js': ` + import { test as setup } from '@playwright/test'; + setup('login once', async ({}) => { + await setup.step('login step', async () => {}); + }); + `, 'a.test.js': ` import { test, expect } from '@playwright/test'; test('math 1', async ({}) => { @@ -202,14 +212,24 @@ test('should merge into html', async ({ runInlineTest, mergeReports, showReport, await showReport(); - await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('10'); - await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('3'); + await expect(page.locator('.subnav-item:has-text("All") .counter')).toHaveText('13'); + await expect(page.locator('.subnav-item:has-text("Passed") .counter')).toHaveText('6'); await expect(page.locator('.subnav-item:has-text("Failed") .counter')).toHaveText('2'); await expect(page.locator('.subnav-item:has-text("Flaky") .counter')).toHaveText('2'); await expect(page.locator('.subnav-item:has-text("Skipped") .counter')).toHaveText('3'); - await expect(page.locator('.test-file-test .test-file-title')).toHaveText( - ['failing 1', 'flaky 1', 'math 1', 'skipped 1', 'failing 2', 'math 2', 'skipped 2', 'flaky 2', 'math 3', 'skipped 3']); + await expect(page.locator('.test-file-test .test-file-title')).toHaveText([ + 'failing 1', 'flaky 1', 'math 1', 'skipped 1', + 'failing 2', 'math 2', 'skipped 2', + 'flaky 2', 'math 3', 'skipped 3', + 'login once', 'login once', 'login once', + ]); + + for (let i = 0; i < 3; i++) { + await page.getByText('login once').nth(i).click(); + await expect(page.getByText('login step')).toBeVisible(); + await page.goBack(); + } }); test('be able to merge incomplete shards', async ({ runInlineTest, mergeReports, showReport, page }) => {