From 039e87f5cbba5fba09b3aa984bee267f82bc535d Mon Sep 17 00:00:00 2001 From: Adam Gastineau Date: Wed, 16 Apr 2025 13:03:45 -0700 Subject: [PATCH] fix(blob): properly communicate attachments through blob merges (#35517) --- .../playwright/src/isomorphic/teleReceiver.ts | 30 ++++++- packages/playwright/src/reporters/merge.ts | 24 +++-- .../playwright/src/reporters/teleEmitter.ts | 29 ++++++- tests/playwright-test/reporter-blob.spec.ts | 87 +++++++++++++++++++ 4 files changed, 160 insertions(+), 10 deletions(-) diff --git a/packages/playwright/src/isomorphic/teleReceiver.ts b/packages/playwright/src/isomorphic/teleReceiver.ts index 712fd4edaa..069ddb4a43 100644 --- a/packages/playwright/src/isomorphic/teleReceiver.ts +++ b/packages/playwright/src/isomorphic/teleReceiver.ts @@ -87,14 +87,15 @@ export type JsonTestResultStart = { startTime: number; }; -export type JsonAttachment = Omit & { base64?: string }; +export type JsonAttachment = Omit & { base64?: string; }; export type JsonTestResultEnd = { id: string; duration: number; status: reporterTypes.TestStatus; errors: reporterTypes.TestError[]; - attachments: JsonAttachment[]; + /** No longer emitted, but kept for backwards compatibility */ + attachments?: JsonAttachment[]; annotations?: Annotation[]; }; @@ -115,6 +116,12 @@ export type JsonTestStepEnd = { annotations?: Annotation[]; }; +export type JsonTestResultOnAttach = { + testId: string; + resultId: string; + attachments: JsonAttachment[]; +}; + export type JsonFullResult = { status: reporterTypes.FullResult['status']; startTime: number; @@ -180,6 +187,10 @@ export class TeleReporterReceiver { this._onStepBegin(params.testId, params.resultId, params.step); return; } + if (method === 'onAttach') { + this._onAttach(params.testId, params.resultId, params.attachments); + return; + } if (method === 'onStepEnd') { this._onStepEnd(params.testId, params.resultId, params.step); return; @@ -241,7 +252,9 @@ export class TeleReporterReceiver { result.status = payload.status; result.errors = payload.errors; result.error = result.errors?.[0]; - result.attachments = this._parseAttachments(payload.attachments); + // Attachments are only present here from legacy blobs. These override all _onAttach events + if (!!payload.attachments) + result.attachments = this._parseAttachments(payload.attachments); if (payload.annotations) { result.annotations = payload.annotations; test.annotations = result.annotations; @@ -276,6 +289,17 @@ export class TeleReporterReceiver { this._reporter.onStepEnd?.(test, result, step); } + private _onAttach(testId: string, resultId: string, attachments: JsonAttachment[]) { + const test = this._tests.get(testId)!; + const result = test.results.find(r => r._id === resultId)!; + result.attachments.push(...attachments.map(a => ({ + name: a.name, + contentType: a.contentType, + path: a.path, + body: a.base64 && (globalThis as any).Buffer ? Buffer.from(a.base64, 'base64') : undefined, + }))); + } + private _onError(error: reporterTypes.TestError) { this._reporter.onError?.(error); } diff --git a/packages/playwright/src/reporters/merge.ts b/packages/playwright/src/reporters/merge.ts index 9e2bd2f5df..5fe408b929 100644 --- a/packages/playwright/src/reporters/merge.ts +++ b/packages/playwright/src/reporters/merge.ts @@ -30,7 +30,7 @@ import type { BlobReportMetadata } from './blob'; import type { ReporterDescription } from '../../types/test'; import type { TestError } from '../../types/testReporter'; import type { FullConfigInternal } from '../common/config'; -import type { JsonConfig, JsonEvent, JsonFullResult, JsonLocation, JsonProject, JsonSuite, JsonTestCase, JsonTestResultEnd, JsonTestStepEnd, JsonTestStepStart } from '../isomorphic/teleReceiver'; +import type { JsonAttachment, JsonConfig, JsonEvent, JsonFullResult, JsonLocation, JsonProject, JsonSuite, JsonTestCase, JsonTestResultEnd, JsonTestResultOnAttach, JsonTestStepEnd, JsonTestStepStart } from '../isomorphic/teleReceiver'; import type * as blobV1 from './versions/blobV1'; type StatusCallback = (message: string) => void; @@ -390,6 +390,7 @@ class IdsPatcher { case 'onProject': this._onProject(params.project); return; + case 'onAttach': case 'onTestBegin': case 'onStepBegin': case 'onStepEnd': @@ -447,9 +448,14 @@ class AttachmentPathPatcher { } patchEvent(event: JsonEvent) { - if (event.method !== 'onTestEnd') - return; - for (const attachment of (event.params.result as JsonTestResultEnd).attachments) { + if (event.method === 'onAttach') + this._patchAttachments((event.params as JsonTestResultOnAttach).attachments); + else if (event.method === 'onTestEnd') + this._patchAttachments((event.params as JsonTestResultEnd).attachments ?? []); + } + + private _patchAttachments(attachments: JsonAttachment[]) { + for (const attachment of attachments) { if (!attachment.path) continue; @@ -476,7 +482,7 @@ class PathSeparatorPatcher { if (jsonEvent.method === 'onTestEnd') { const testResult = jsonEvent.params.result as JsonTestResultEnd; testResult.errors.forEach(error => this._updateErrorLocations(error)); - testResult.attachments.forEach(attachment => { + (testResult.attachments ?? []).forEach(attachment => { if (attachment.path) attachment.path = this._updatePath(attachment.path); }); @@ -492,6 +498,14 @@ class PathSeparatorPatcher { this._updateErrorLocations(step.error); return; } + if (jsonEvent.method === 'onAttach') { + const attach = jsonEvent.params as JsonTestResultOnAttach; + attach.attachments.forEach(attachment => { + if (attachment.path) + attachment.path = this._updatePath(attachment.path); + }); + return; + } } private _updateProject(project: JsonProject) { diff --git a/packages/playwright/src/reporters/teleEmitter.ts b/packages/playwright/src/reporters/teleEmitter.ts index 2d00e3264a..b403cab303 100644 --- a/packages/playwright/src/reporters/teleEmitter.ts +++ b/packages/playwright/src/reporters/teleEmitter.ts @@ -33,6 +33,7 @@ export class TeleReporterEmitter implements ReporterV2 { private _messageSink: (message: teleReceiver.JsonEvent) => void; private _rootDir!: string; private _emitterOptions: TeleReporterEmitterOptions; + private _resultKnownAttachmentCounts = new Map(); // In case there is blob reporter and UI mode, make sure one does override // the id assigned by the other. private readonly _idSymbol = Symbol('id'); @@ -76,6 +77,7 @@ export class TeleReporterEmitter implements ReporterV2 { timeout: test.timeout, annotations: [] }; + this._sendNewAttachments(result, test.id); this._messageSink({ method: 'onTestEnd', params: { @@ -83,6 +85,8 @@ export class TeleReporterEmitter implements ReporterV2 { result: this._serializeResultEnd(result), } }); + + this._resultKnownAttachmentCounts.delete((result as any)[this._idSymbol]); } onStepBegin(test: reporterTypes.TestCase, result: reporterTypes.TestResult, step: reporterTypes.TestStep): void { @@ -98,11 +102,15 @@ export class TeleReporterEmitter implements ReporterV2 { } onStepEnd(test: reporterTypes.TestCase, result: reporterTypes.TestResult, step: reporterTypes.TestStep): void { + // Create synthetic onAttach event so we serialize the entire attachment along with the step + const resultId = (result as any)[this._idSymbol] as string; + this._sendNewAttachments(result, test.id); + this._messageSink({ method: 'onStepEnd', params: { testId: test.id, - resultId: (result as any)[this._idSymbol], + resultId, step: this._serializeStepEnd(step, result) } }); @@ -236,11 +244,28 @@ export class TeleReporterEmitter implements ReporterV2 { duration: result.duration, status: result.status, errors: result.errors, - attachments: this._serializeAttachments(result.attachments), annotations: result.annotations?.length ? result.annotations : undefined, }; } + private _sendNewAttachments(result: reporterTypes.TestResult, testId: string) { + const resultId = (result as any)[this._idSymbol] as string; + // Track whether this step (or something else since the last step) has added attachments and send them + const knownAttachmentCount = this._resultKnownAttachmentCounts.get(resultId) ?? 0; + if (result.attachments.length > knownAttachmentCount) { + this._messageSink({ + method: 'onAttach', + params: { + testId, + resultId, + attachments: this._serializeAttachments((result.attachments.slice(knownAttachmentCount))), + } + }); + } + + this._resultKnownAttachmentCounts.set(resultId, result.attachments.length); + } + _serializeAttachments(attachments: reporterTypes.TestResult['attachments']): teleReceiver.JsonAttachment[] { return attachments.map(a => { const { body, ...rest } = a; diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index 9b4f77fa17..504025f7de 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -25,6 +25,7 @@ import extractZip from '../../packages/playwright-core/bundles/zip/node_modules/ import * as yazl from '../../packages/playwright-core/bundles/zip/node_modules/yazl'; import { getUserAgent } from '../../packages/playwright-core/lib/server/utils/userAgent'; import { Readable } from 'stream'; +import type { JSONReportTestResult } from '../../packages/playwright-test/reporter'; const DOES_NOT_SUPPORT_UTF8_IN_TERMINAL = process.platform === 'win32' && process.env.TERM_PROGRAM !== 'vscode' && !process.env.WT_SESSION; const POSITIVE_STATUS_MARK = DOES_NOT_SUPPORT_UTF8_IN_TERMINAL ? 'ok' : '✓ '; @@ -1824,6 +1825,92 @@ test('merge reports without --config preserves path separators', async ({ runInl expect(output).toContain(`test title: ${'tests2' + otherSeparator + 'b.test.js'}`); }); +test('merge reports should preserve attachments', async ({ runInlineTest, mergeReports, showReport, page }) => { + const reportDir = test.info().outputPath('blob-report-orig'); + const files = { + 'playwright.config.ts': ` + module.exports = { + retries: 1, + reporter: [['blob', { outputDir: '${reportDir.replace(/\\/g, '/')}' }]] + }; + `, + 'a.test.js': ` + import { test, expect } from '@playwright/test'; + import * as fs from 'fs'; + test('attachment A', async ({}) => { + const attachmentPath = test.info().outputPath('foo.txt'); + fs.writeFileSync(attachmentPath, 'hello!'); + await test.info().attach('file-attachment1', { path: attachmentPath }); + await test.info().attachments.push({ name: 'file-attachment2', path: attachmentPath, contentType: 'text/html' }); + await test.info().attach('file-attachment3', { path: attachmentPath }); + await test.info().attach('file-attachment4', { path: attachmentPath }); + await test.info().attachments.push({ name: 'file-attachment5', path: attachmentPath, contentType: 'text/html' }); + await test.info().attachments.push({ name: 'file-attachment6', path: attachmentPath, contentType: 'text/html' }); + await test.info().attach('file-attachment7', { path: attachmentPath }); + }); + `, + 'b.test.js': ` + import { test, expect } from '@playwright/test'; + import * as fs from 'fs'; + test('attachment B', async ({}) => { + const attachmentPath = test.info().outputPath('bar.txt'); + fs.writeFileSync(attachmentPath, 'goodbye!'); + await test.info().attach('file-attachment8', { path: attachmentPath }); + await test.info().attachments.push({ name: 'file-attachment9', path: attachmentPath, contentType: 'application/json' }); + }); + ` + }; + await runInlineTest(files, { shard: `1/2` }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' }); + await runInlineTest(files, { shard: `2/2` }, { PWTEST_BLOB_DO_NOT_REMOVE: '1' }); + + const reportFiles = await fs.promises.readdir(reportDir); + reportFiles.sort(); + expect(reportFiles).toEqual(['report-1.zip', 'report-2.zip']); + const { exitCode } = await mergeReports(reportDir, { 'PLAYWRIGHT_HTML_OPEN': 'never' }, { additionalArgs: ['--reporter', 'blob,html'] }); + expect(exitCode).toBe(0); + + const reportZipFile = test.info().outputPath('blob-report', 'report.zip'); + const events = await extractReport(reportZipFile, test.info().outputPath('tmp')); + + type Attachment = Omit & { + path: any + }; + + const attachment1: Attachment = { name: 'file-attachment1', path: expect.stringContaining(''), contentType: 'text/plain' }; + const attachment2: Attachment = { name: 'file-attachment2', path: expect.stringContaining(''), contentType: 'text/html' }; + const attachment3: Attachment = { name: 'file-attachment3', path: expect.stringContaining(''), contentType: 'text/plain' }; + const attachment4: Attachment = { name: 'file-attachment4', path: expect.stringContaining(''), contentType: 'text/plain' }; + const attachment5: Attachment = { name: 'file-attachment5', path: expect.stringContaining(''), contentType: 'text/html' }; + const attachment6: Attachment = { name: 'file-attachment6', path: expect.stringContaining(''), contentType: 'text/html' }; + const attachment7: Attachment = { name: 'file-attachment7', path: expect.stringContaining(''), contentType: 'text/plain' }; + const attachment8: Attachment = { name: 'file-attachment8', path: expect.stringContaining(''), contentType: 'text/plain' }; + const attachment9: Attachment = { name: 'file-attachment9', path: expect.stringContaining(''), contentType: 'application/json' }; + + const aAttachments = [attachment1, attachment2, attachment3, attachment4, attachment5, attachment6, attachment7]; + const bAttachments = [attachment8, attachment9]; + + const allStepAttachments = events.flatMap(e => e.method === 'onStepEnd' ? e?.params?.step?.attachments ?? [] : []); + expect(allStepAttachments).toEqual([0, 2, 3, 6, 0]); + + const allTestAttachments = events.flatMap(e => e.method === 'onAttach' ? e?.params?.attachments ?? [] : []); + expect(allTestAttachments).toEqual([...aAttachments, ...bAttachments]); + + await showReport(); + + { + await page.getByRole('link', { name: 'Attachment A' }).click(); + for (const attachment of aAttachments) + await expect(page.getByRole('link', { name: attachment.name })).toBeVisible(); + await page.goBack(); + } + { + await page.getByRole('link', { name: 'Attachment B' }).click(); + for (const attachment of bAttachments) + await expect(page.getByRole('link', { name: attachment.name })).toBeVisible(); + await page.goBack(); + } +}); + test('merge reports must not change test ids when there is no need to', async ({ runInlineTest, mergeReports }) => { const files = { 'echo-test-id-reporter.js': `