From 80b9e0283708997753e4f495ad638eb36d016914 Mon Sep 17 00:00:00 2001
From: Pavel Feldman <pavel.feldman@gmail.com>
Date: Fri, 8 Sep 2023 18:00:12 -0700
Subject: [PATCH] fix(trace): do not attach screenshots twice (#26971)

---
 packages/playwright/src/worker/testInfo.ts    | 13 ++++---------
 packages/playwright/src/worker/testTracing.ts | 10 +++++-----
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/packages/playwright/src/worker/testInfo.ts b/packages/playwright/src/worker/testInfo.ts
index 81d3fb4d90..bb0e475eb6 100644
--- a/packages/playwright/src/worker/testInfo.ts
+++ b/packages/playwright/src/worker/testInfo.ts
@@ -25,9 +25,10 @@ import type { Annotation, FullConfigInternal, FullProjectInternal } from '../com
 import type { Location } from '../../types/testReporter';
 import { getContainedPath, normalizeAndSaveAttachment, serializeError, trimLongString } from '../util';
 import { TestTracing } from './testTracing';
+import type { Attachment } from './testTracing';
 
 export interface TestStepInternal {
-  complete(result: { error?: Error | TestInfoError }): void;
+  complete(result: { error?: Error | TestInfoError, attachments?: Attachment[] }): void;
   stepId: string;
   title: string;
   category: string;
@@ -261,8 +262,6 @@ export class TestInfoImpl implements TestInfo {
       isLaxParent = !!parentStep;
     }
 
-    const initialAttachments = new Set(this.attachments);
-
     const step: TestStepInternal = {
       stepId,
       ...data,
@@ -304,7 +303,7 @@ export class TestInfoImpl implements TestInfo {
         };
         this._onStepEnd(payload);
         const errorForTrace = error ? { name: '', message: error.message || '', stack: error.stack } : undefined;
-        this._tracing.appendAfterActionForStep(stepId, this.attachments, initialAttachments, errorForTrace);
+        this._tracing.appendAfterActionForStep(stepId, errorForTrace, result.attachments);
       }
     };
     const parentStepList = parentStep ? parentStep.steps : this._steps;
@@ -380,11 +379,6 @@ export class TestInfoImpl implements TestInfo {
       wallTime: Date.now(),
       laxParent: true,
     });
-    this._attachWithoutStep(attachment);
-    step.complete({});
-  }
-
-  _attachWithoutStep(attachment: TestInfo['attachments'][0]) {
     this._attachmentsPush(attachment);
     this._onAttach({
       testId: this._test.id,
@@ -393,6 +387,7 @@ export class TestInfoImpl implements TestInfo {
       path: attachment.path,
       body: attachment.body?.toString('base64')
     });
+    step.complete({ attachments: [attachment] });
   }
 
   outputPath(...pathSegments: string[]){
diff --git a/packages/playwright/src/worker/testTracing.ts b/packages/playwright/src/worker/testTracing.ts
index c671f5536c..c6d8007d1e 100644
--- a/packages/playwright/src/worker/testTracing.ts
+++ b/packages/playwright/src/worker/testTracing.ts
@@ -24,7 +24,7 @@ import { yauzl, yazl } from 'playwright-core/lib/zipBundle';
 import type { TestInfo } from '../../types/test';
 
 
-type Attachment = TestInfo['attachments'][0];
+export type Attachment = TestInfo['attachments'][0];
 export const testTraceEntryName = 'test.trace';
 
 export class TestTracing {
@@ -125,13 +125,13 @@ export class TestTracing {
     });
   }
 
-  appendAfterActionForStep(callId: string, attachments: Attachment[], initialAttachments: Set<Attachment>, error?: SerializedError['error']) {
+  appendAfterActionForStep(callId: string, error?: SerializedError['error'], attachments: Attachment[] = []) {
     this._appendTraceEvent({
       type: 'after',
       callId,
       endTime: monotonicTime(),
       log: [],
-      attachments: serializeAttachments(attachments, initialAttachments),
+      attachments: serializeAttachments(attachments),
       error,
     });
   }
@@ -143,8 +143,8 @@ export class TestTracing {
   }
 }
 
-function serializeAttachments(attachments: Attachment[], initialAttachments: Set<Attachment>): trace.AfterActionTraceEvent['attachments'] {
-  return attachments.filter(a => a.name !== 'trace' && !initialAttachments.has(a)).map(a => {
+function serializeAttachments(attachments: Attachment[]): trace.AfterActionTraceEvent['attachments'] {
+  return attachments.filter(a => a.name !== 'trace').map(a => {
     return {
       name: a.name,
       contentType: a.contentType,