chore: include test-end-screenshot in a trace (#23457)

Fixes https://github.com/microsoft/playwright/issues/23222

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 = {

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
+    // Either remove or attach temporary traces for contexts closed before the
-    // before the test has finished.
+    // 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() {

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()];

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) {

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<TestInfo['attachments'][0]>): 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,

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')
-    }))
   };
 }
 

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);
+}

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 <div className='attachments-tab'>
@@ -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,
     }} />}
-    {<div className='attachments-section'>Attachments</div>}
+    {screenshots.size ? <div className='attachments-section'>Screenshots</div> : undefined}
-    {action.attachments?.map(a => {
+    {[...screenshots].map((a, i) => {
-      return <div className='attachment-item'>
+      return <div className='attachment-item' key={`screenshot-${i}`}>
+        <div><img src={attachmentURL(traceUrl, a)} /></div>
+        <div><a target='_blank' href={attachmentURL(traceUrl, a)}>{a.name}</a></div>
+      </div>;
+    })}
+    {otherAttachments.size ? <div className='attachments-section'>Attachments</div> : undefined}
+    {[...otherAttachments].map((a, i) => {
+      return <div className='attachment-item' key={`attachment-${i}`}>
         <a target='_blank' href={attachmentURL(traceUrl, a)}>{a.name}</a>
       </div>;
     })}

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',

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();

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.getByText('passing', { exact: true }).click();
-  await page.locator('text=example.txt').click();
+  await page.getByText('example.txt', { exact: true }).click();
-  await page.locator('text=example.json').click();
+  await page.getByText('example.json', { exact: true }).click();
-  await page.locator('text=example-utf16.txt').click();
+  await page.getByText('example-utf16.txt', { exact: true }).click();
   await expect(page.locator('.attachment-body')).toHaveText(['foo', '{"foo":1}', 'utf16 encoded']);
 });
 

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);
+});