From d1666d2dde999256c5f09505d4d49e3b6ccb3cdd Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 1 Jun 2023 20:29:32 -0700 Subject: [PATCH] chore: include test-end-screenshot in a trace (#23457) Fixes https://github.com/microsoft/playwright/issues/23222 --- packages/playwright-test/src/common/ipc.ts | 9 +++++- packages/playwright-test/src/index.ts | 19 ++++++----- .../playwright-test/src/runner/dispatcher.ts | 22 +++++++++---- packages/playwright-test/src/util.ts | 2 +- .../playwright-test/src/worker/testInfo.ts | 32 +++++++++++++++++-- .../playwright-test/src/worker/workerMain.ts | 9 ++---- .../trace-viewer/src/ui/attachmentsTab.css | 7 ++++ .../trace-viewer/src/ui/attachmentsTab.tsx | 17 ++++++++-- .../playwright-test/playwright.trace.spec.ts | 1 + tests/playwright-test/reporter-blob.spec.ts | 9 +++--- tests/playwright-test/reporter-html.spec.ts | 8 ++--- tests/playwright-test/ui-mode-trace.spec.ts | 23 +++++++++++++ 12 files changed, 120 insertions(+), 38 deletions(-) diff --git a/packages/playwright-test/src/common/ipc.ts b/packages/playwright-test/src/common/ipc.ts index c662fe4589..5536426d56 100644 --- a/packages/playwright-test/src/common/ipc.ts +++ b/packages/playwright-test/src/common/ipc.ts @@ -69,6 +69,14 @@ export type TestBeginPayload = { startWallTime: number; // milliseconds since unix epoch }; +export type AttachmentPayload = { + testId: string; + name: string; + path?: string; + body?: string; + contentType: string; +}; + export type TestEndPayload = { testId: string; duration: number; @@ -77,7 +85,6 @@ export type TestEndPayload = { expectedStatus: TestStatus; annotations: { type: string, description?: string }[]; timeout: number; - attachments: { name: string, path?: string, body?: string, contentType: string }[]; }; export type StepBeginPayload = { diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index d74cc45c09..bb90938546 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -627,6 +627,15 @@ class ArtifactsRecorder { await this._stopTracing(tracing, true); }))); + // Either remove or attach temporary screenshots for contexts closed before + // collecting the test trace. + await Promise.all(this._temporaryScreenshots.map(async file => { + if (captureScreenshots) + await fs.promises.rename(file, this._addScreenshotAttachment()).catch(() => {}); + else + await fs.promises.unlink(file).catch(() => {}); + })); + // Collect test trace. if (this._preserveTrace()) { const events = this._testInfo._traceEvents; @@ -643,8 +652,8 @@ class ArtifactsRecorder { } } - // Either remove or attach temporary traces and screenshots for contexts closed - // before the test has finished. + // Either remove or attach temporary traces for contexts closed before the + // test has finished. if (this._preserveTrace() && this._temporaryTraceFiles.length) { const tracePath = this._testInfo.outputPath(`trace.zip`); // This could be: beforeHooks, or beforeHooks + test, etc. @@ -658,12 +667,6 @@ class ArtifactsRecorder { if (!beforeHooksHadTrace) this._testInfo.attachments.push({ name: 'trace', path: tracePath, contentType: 'application/zip' }); } - await Promise.all(this._temporaryScreenshots.map(async file => { - if (captureScreenshots) - await fs.promises.rename(file, this._addScreenshotAttachment()).catch(() => {}); - else - await fs.promises.unlink(file).catch(() => {}); - })); } private _addScreenshotAttachment() { diff --git a/packages/playwright-test/src/runner/dispatcher.ts b/packages/playwright-test/src/runner/dispatcher.ts index e76b7581a2..434570914e 100644 --- a/packages/playwright-test/src/runner/dispatcher.ts +++ b/packages/playwright-test/src/runner/dispatcher.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import type { TestBeginPayload, TestEndPayload, DonePayload, TestOutputPayload, StepBeginPayload, StepEndPayload, TeardownErrorsPayload, RunPayload, SerializedConfig } from '../common/ipc'; +import type { TestBeginPayload, TestEndPayload, DonePayload, TestOutputPayload, StepBeginPayload, StepEndPayload, TeardownErrorsPayload, RunPayload, SerializedConfig, AttachmentPayload } from '../common/ipc'; import { serializeConfig } from '../common/ipc'; import type { TestResult, TestStep, TestError } from '../../types/testReporter'; import type { Suite } from '../common/test'; @@ -207,6 +207,7 @@ export class Dispatcher { worker.removeListener('testEnd', onTestEnd); worker.removeListener('stepBegin', onStepBegin); worker.removeListener('stepEnd', onStepEnd); + worker.removeListener('attach', onAttach); worker.removeListener('done', onDone); worker.removeListener('exit', onExit); doneCallback(); @@ -245,12 +246,6 @@ export class Dispatcher { result.duration = params.duration; result.errors = params.errors; result.error = result.errors[0]; - result.attachments = params.attachments.map(a => ({ - name: a.name, - path: a.path, - contentType: a.contentType, - body: a.body !== undefined ? Buffer.from(a.body, 'base64') : undefined - })); result.status = params.status; test.expectedStatus = params.expectedStatus; test.annotations = params.annotations; @@ -312,6 +307,19 @@ export class Dispatcher { }; worker.on('stepEnd', onStepEnd); + const onAttach = (params: AttachmentPayload) => { + const data = this._testById.get(params.testId)!; + const { result } = data.resultByWorkerIndex.get(worker.workerIndex)!; + const attachment = { + name: params.name, + path: params.path, + contentType: params.contentType, + body: params.body !== undefined ? Buffer.from(params.body, 'base64') : undefined + }; + result.attachments.push(attachment); + }; + worker.on('attach', onAttach); + const onDone = (params: DonePayload & { unexpectedExitError?: TestError }) => { this._queuedOrRunningHashCount.set(worker.hash(), this._queuedOrRunningHashCount.get(worker.hash())! - 1); let remaining = [...remainingByTestId.values()]; diff --git a/packages/playwright-test/src/util.ts b/packages/playwright-test/src/util.ts index c5d2b6aae3..3cd88fbf81 100644 --- a/packages/playwright-test/src/util.ts +++ b/packages/playwright-test/src/util.ts @@ -261,7 +261,7 @@ export function getPackageJsonPath(folderPath: string): string { return result; } -export async function normalizeAndSaveAttachment(outputPath: string, name: string, options: { path?: string, body?: string | Buffer, contentType?: string } = {}): Promise<{ name: string; path?: string | undefined; body?: Buffer | undefined; contentType: string; }> { +export async function normalizeAndSaveAttachment(outputPath: string, name: string, options: { path?: string, body?: string | Buffer, contentType?: string } = {}): Promise<{ name: string; path?: string | undefined; body?: Buffer | undefined; contentType: string; }> { if ((options.path !== undefined ? 1 : 0) + (options.body !== undefined ? 1 : 0) !== 1) throw new Error(`Exactly one of "path" and "body" must be specified`); if (options.path !== undefined) { diff --git a/packages/playwright-test/src/worker/testInfo.ts b/packages/playwright-test/src/worker/testInfo.ts index db2c7bf1ed..e363a09a72 100644 --- a/packages/playwright-test/src/worker/testInfo.ts +++ b/packages/playwright-test/src/worker/testInfo.ts @@ -18,7 +18,7 @@ import fs from 'fs'; import path from 'path'; import { captureRawStack, createAfterActionTraceEventForStep, createBeforeActionTraceEventForStep, monotonicTime, zones } from 'playwright-core/lib/utils'; import type { TestInfoError, TestInfo, TestStatus, FullProject, FullConfig } from '../../types/test'; -import type { StepBeginPayload, StepEndPayload, WorkerInitParams } from '../common/ipc'; +import type { AttachmentPayload, StepBeginPayload, StepEndPayload, WorkerInitParams } from '../common/ipc'; import type { TestCase } from '../common/test'; import { TimeoutManager } from './timeoutManager'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; @@ -45,6 +45,7 @@ export interface TestStepInternal { export class TestInfoImpl implements TestInfo { private _onStepBegin: (payload: StepBeginPayload) => void; private _onStepEnd: (payload: StepEndPayload) => void; + private _onAttach: (payload: AttachmentPayload) => void; readonly _test: TestCase; readonly _timeoutManager: TimeoutManager; readonly _startTime: number; @@ -86,6 +87,7 @@ export class TestInfoImpl implements TestInfo { readonly outputDir: string; readonly snapshotDir: string; errors: TestInfoError[] = []; + private _attachmentsPush: (...items: TestInfo['attachments']) => number; get error(): TestInfoError | undefined { return this.errors[0]; @@ -113,11 +115,13 @@ export class TestInfoImpl implements TestInfo { retry: number, onStepBegin: (payload: StepBeginPayload) => void, onStepEnd: (payload: StepEndPayload) => void, + onAttach: (payload: AttachmentPayload) => void, ) { this._test = test; this.testId = test.id; this._onStepBegin = onStepBegin; this._onStepEnd = onStepEnd; + this._onAttach = onAttach; this._startTime = monotonicTime(); this._startWallTime = Date.now(); @@ -158,6 +162,13 @@ export class TestInfoImpl implements TestInfo { const relativeTestFilePath = path.relative(this.project.testDir, test._requireFile); return path.join(this.project.snapshotDir, relativeTestFilePath + '-snapshots'); })(); + + this._attachmentsPush = this.attachments.push.bind(this.attachments); + this.attachments.push = (...attachments: TestInfo['attachments']) => { + for (const a of attachments) + this._attach(a.name, a); + return this.attachments.length; + }; } private _modifier(type: 'skip' | 'fail' | 'fixme' | 'slow', modifierArgs: [arg?: any, description?: string]) { @@ -333,15 +344,30 @@ export class TestInfoImpl implements TestInfo { // ------------ TestInfo methods ------------ async attach(name: string, options: { path?: string, body?: string | Buffer, contentType?: string } = {}) { + this._attach(name, await normalizeAndSaveAttachment(this.outputPath(), name, options)); + } + + private _attach(name: string, attachment: TestInfo['attachments'][0]) { const step = this._addStep({ title: `attach "${name}"`, category: 'attach', wallTime: Date.now(), }); - this.attachments.push(await normalizeAndSaveAttachment(this.outputPath(), name, options)); + this._attachWithoutStep(attachment); step.complete({}); } + _attachWithoutStep(attachment: TestInfo['attachments'][0]) { + this._attachmentsPush(attachment); + this._onAttach({ + testId: this._test.id, + name: attachment.name, + contentType: attachment.contentType, + path: attachment.path, + body: attachment.body?.toString('base64') + }); + } + outputPath(...pathSegments: string[]){ fs.mkdirSync(this.outputDir, { recursive: true }); const joinedPath = path.join(...pathSegments); @@ -401,7 +427,7 @@ export class TestInfoImpl implements TestInfo { } function serializeAttachments(attachments: TestInfo['attachments'], initialAttachments: Set): trace.AfterActionTraceEvent['attachments'] { - return attachments.filter(a => !initialAttachments.has(a)).map(a => { + return attachments.filter(a => a.name !== 'trace' && !initialAttachments.has(a)).map(a => { return { name: a.name, contentType: a.contentType, diff --git a/packages/playwright-test/src/worker/workerMain.ts b/packages/playwright-test/src/worker/workerMain.ts index 827cd20a55..ef761641c6 100644 --- a/packages/playwright-test/src/worker/workerMain.ts +++ b/packages/playwright-test/src/worker/workerMain.ts @@ -248,7 +248,8 @@ export class WorkerMain extends ProcessRunner { private async _runTest(test: TestCase, retry: number, nextTest: TestCase | undefined) { const testInfo = new TestInfoImpl(this._config, this._project, this._params, test, retry, stepBeginPayload => this.dispatchEvent('stepBegin', stepBeginPayload), - stepEndPayload => this.dispatchEvent('stepEnd', stepEndPayload)); + stepEndPayload => this.dispatchEvent('stepEnd', stepEndPayload), + attachment => this.dispatchEvent('attach', attachment)); const processAnnotation = (annotation: Annotation) => { testInfo.annotations.push(annotation); @@ -601,12 +602,6 @@ function buildTestEndPayload(testInfo: TestInfoImpl): TestEndPayload { expectedStatus: testInfo.expectedStatus, annotations: testInfo.annotations, timeout: testInfo.timeout, - attachments: testInfo.attachments.map(a => ({ - name: a.name, - contentType: a.contentType, - path: a.path, - body: a.body?.toString('base64') - })) }; } diff --git a/packages/trace-viewer/src/ui/attachmentsTab.css b/packages/trace-viewer/src/ui/attachmentsTab.css index 46eae4326b..43291d5f1a 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.css +++ b/packages/trace-viewer/src/ui/attachmentsTab.css @@ -38,3 +38,10 @@ .attachment-item { margin: 4px 8px; } + +.attachment-item img { + flex: none; + min-width: 200px; + max-width: 80%; + box-shadow: 0 12px 28px 0 rgba(0,0,0,.2), 0 2px 4px 0 rgba(0,0,0,.1); +} diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx index f118751aa3..14b066979e 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.tsx +++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx @@ -28,6 +28,10 @@ export const AttachmentsTab: React.FunctionComponent<{ const expected = action.attachments?.find(a => a.name.endsWith('-expected.png') && (a.path || a.sha1)) as TestAttachment | undefined; const actual = action.attachments?.find(a => a.name.endsWith('-actual.png') && (a.path || a.sha1)) as TestAttachment | undefined; const diff = action.attachments?.find(a => a.name.endsWith('-diff.png') && (a.path || a.sha1)) as TestAttachment | undefined; + const screenshots = new Set(action.attachments?.filter(a => a.contentType.startsWith('image/'))); + const otherAttachments = new Set(action.attachments || []); + screenshots.forEach(a => otherAttachments.delete(a)); + const traceUrl = action.context.traceUrl; return
@@ -38,9 +42,16 @@ export const AttachmentsTab: React.FunctionComponent<{ actual: { attachment: { ...actual, path: attachmentURL(traceUrl, actual) } }, diff: diff ? { attachment: { ...diff, path: attachmentURL(traceUrl, diff) } } : undefined, }} />} - {
Attachments
} - {action.attachments?.map(a => { - return
+ {screenshots.size ?
Screenshots
: undefined} + {[...screenshots].map((a, i) => { + return
+
+ +
; + })} + {otherAttachments.size ?
Attachments
: undefined} + {[...otherAttachments].map((a, i) => { + return
{a.name}
; })} diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index bd95a7c3aa..49af7babbd 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -334,6 +334,7 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve 'After Hooks', 'fixture: page', 'fixture: context', + 'attach \"trace\"', 'afterAll hook', 'fixture: request', 'apiRequest.newContext', diff --git a/tests/playwright-test/reporter-blob.spec.ts b/tests/playwright-test/reporter-blob.spec.ts index 9f8c3e6709..5c1aa0404a 100644 --- a/tests/playwright-test/reporter-blob.spec.ts +++ b/tests/playwright-test/reporter-blob.spec.ts @@ -547,13 +547,14 @@ test('resource names should not clash between runs', async ({ runInlineTest, sho await showReport(); + const fileAttachment = page.getByRole('link', { name: 'file-attachment' }); // Check first attachment content. { await page.getByText('first').click(); - await expect(page.getByText('file-attachment')).toBeVisible(); + await expect(fileAttachment).toBeVisible(); const popupPromise = page.waitForEvent('popup'); - await page.getByText('file-attachment').click(); + await fileAttachment.click(); const popup = await popupPromise; await expect(popup.locator('body')).toHaveText('hello!'); await popup.close(); @@ -563,10 +564,10 @@ test('resource names should not clash between runs', async ({ runInlineTest, sho // Check second attachment content. { await page.getByText('failing 2').click(); - await expect(page.getByText('file-attachment')).toBeVisible(); + await expect(fileAttachment).toBeVisible(); const popupPromise = page.waitForEvent('popup'); - await page.getByText('file-attachment').click(); + await fileAttachment.click(); const popup = await popupPromise; await expect(popup.locator('body')).toHaveText('bye!'); await popup.close(); diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index 0092315da5..89e79a32bb 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -673,10 +673,10 @@ test('should render text attachments as text', async ({ runInlineTest, page, sho expect(result.exitCode).toBe(0); await showReport(); - await page.locator('text=passing').click(); - await page.locator('text=example.txt').click(); - await page.locator('text=example.json').click(); - await page.locator('text=example-utf16.txt').click(); + await page.getByText('passing', { exact: true }).click(); + await page.getByText('example.txt', { exact: true }).click(); + await page.getByText('example.json', { exact: true }).click(); + await page.getByText('example-utf16.txt', { exact: true }).click(); await expect(page.locator('.attachment-body')).toHaveText(['foo', '{"foo":1}', 'utf16 encoded']); }); diff --git a/tests/playwright-test/ui-mode-trace.spec.ts b/tests/playwright-test/ui-mode-trace.spec.ts index d8409d7ded..3ddc15cf59 100644 --- a/tests/playwright-test/ui-mode-trace.spec.ts +++ b/tests/playwright-test/ui-mode-trace.spec.ts @@ -182,3 +182,26 @@ test('should show image diff', async ({ runUITest, server }) => { await expect(page.getByText('Expected', { exact: true })).toBeVisible(); await expect(page.locator('.image-diff-view .image-wrapper img')).toBeVisible(); }); + +test('should show screenshot', async ({ runUITest, server }) => { + const { page } = await runUITest({ + 'playwright.config.js': ` + module.exports = { + use: { + screenshot: 'on', + viewport: { width: 100, height: 100 } + } + }; + `, + 'a.test.ts': ` + import { test, expect } from '@playwright/test'; + test('vrt test', async ({ page }) => { + }); + `, + }); + + await page.getByText('vrt test').dblclick(); + await page.getByText(/Attachments/).click(); + await expect(page.getByText('Screenshots', { exact: true })).toBeVisible(); + await expect(page.locator('.attachment-item img')).toHaveCount(1); +});