chore(ui): hide top-level attach actions (#35040)

packages/playwright/src/isomorphic/util.ts

@@ -0,0 +1,17 @@
+/**
+ * Copyright (c) Microsoft Corporation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+export const kTopLevelAttachmentPrefix = '_attach';

packages/playwright/src/worker/DEPS.list

@@ -4,3 +4,4 @@
 ../util.ts
 ../utilBundle.ts
 ../matchers/**
+../isomorphic/util.ts

packages/playwright/src/worker/testInfo.ts

@@ -414,14 +414,10 @@ export class TestInfoImpl implements TestInfo {
 
   _attach(attachment: TestInfo['attachments'][0], stepId: string | undefined) {
     const index = this._attachmentsPush(attachment) - 1;
-    if (stepId) {
+    if (stepId)
       this._stepMap.get(stepId)!.attachmentIndices.push(index);
-    } else {
+    else
-      // trace viewer has no means of representing attachments outside of a step, so we create an artificial action
+      this._tracing.appendTopLevelAttachment(attachment);
-      const callId = `attach@${++this._lastStepId}`;
-      this._tracing.appendBeforeActionForStep(callId, this._findLastPredefinedStep(this._steps)?.stepId, 'attach', `attach "${attachment.name}"`, undefined, []);
-      this._tracing.appendAfterActionForStep(callId, undefined, [attachment]);
-    }
 
     this._onAttach({
       testId: this.testId,

packages/playwright/src/worker/testTracing.ts

@@ -20,6 +20,7 @@ import path from 'path';
 import { ManualPromise, SerializedFS, calculateSha1, createGuid, monotonicTime } from 'playwright-core/lib/utils';
 import { yauzl, yazl } from 'playwright-core/lib/zipBundle';
 
+import { kTopLevelAttachmentPrefix } from '../isomorphic/util';
 import { filteredStackTrace } from '../util';
 
 import type { TestInfoImpl } from './testInfo';
@@ -47,6 +48,7 @@ export class TestTracing {
   private _tracesDir: string;
   private _contextCreatedEvent: trace.ContextCreatedTraceEvent;
   private _didFinishTestFunctionAndAfterEachHooks = false;
+  private _lastActionId = 0;
 
   constructor(testInfo: TestInfoImpl, artifactsDir: string) {
     this._testInfo = testInfo;
@@ -291,6 +293,16 @@ export class TestTracing {
     });
   }
 
+  appendTopLevelAttachment(attachment: Attachment) {
+    // trace viewer has no means of representing attachments outside of a step,
+    // so we create an artificial action that's hidden in the UI
+    // the alternative would be to have one hidden step at the end with all top-level attachments,
+    // but that would delay useful information in live traces.
+    const callId = `${kTopLevelAttachmentPrefix}@${++this._lastActionId}`;
+    this.appendBeforeActionForStep(callId, undefined, kTopLevelAttachmentPrefix, `${kTopLevelAttachmentPrefix} "${attachment.name}"`, undefined, []);
+    this.appendAfterActionForStep(callId, undefined, [attachment]);
+  }
+
   private _appendTraceEvent(event: trace.TraceEvent) {
     this._traceEvents.push(event);
     if (this._liveTraceFile)

packages/trace-viewer/src/ui/modelUtil.ts

@@ -20,6 +20,7 @@ import type * as trace from '@trace/trace';
 import type { ActionTraceEvent } from '@trace/trace';
 import type { ActionEntry, ContextEntry, PageEntry } from '../types/entries';
 import type { StackFrame } from '@protocol/channels';
+import { kTopLevelAttachmentPrefix } from '@testIsomorphic/util';
 
 const contextSymbol = Symbol('context');
 const nextInContextSymbol = Symbol('next');
@@ -338,6 +339,8 @@ export function buildActionTree(actions: ActionTraceEventInContext[]): { rootIte
 
   const rootItem: ActionTreeItem = { id: '', parent: undefined, children: [] };
   for (const item of itemMap.values()) {
+    if (item.action?.apiName.startsWith(kTopLevelAttachmentPrefix))
+      continue;
     const parent = item.action!.parentId ? itemMap.get(item.action!.parentId) || rootItem : rootItem;
     parent.children.push(item);
     item.parent = parent;

tests/playwright-test/playwright.connect.spec.ts

@@ -223,7 +223,7 @@ test('should record trace', async ({ runInlineTest }) => {
     'After Hooks',
     'fixture: page',
     'fixture: context',
-    'attach "_prompt-0"',
+    '_attach "_prompt-0"',
     'Worker Cleanup',
     'fixture: browser',
   ]);

tests/playwright-test/playwright.trace.spec.ts

@@ -129,7 +129,6 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => {
     '  fixture: context',
     '  fixture: request',
     '    apiRequestContext.dispose',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);
@@ -328,7 +327,6 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve
     'After Hooks',
     '  fixture: page',
     '  fixture: context',
-    '  attach "_prompt-0"',
     '  afterAll hook',
     '    fixture: request',
     '      apiRequest.newContext',
@@ -671,7 +669,6 @@ test('should show non-expect error in trace', async ({ runInlineTest }, testInfo
     'After Hooks',
     '  fixture: page',
     '  fixture: context',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);
@@ -734,7 +731,7 @@ test('should not throw when attachment is missing', async ({ runInlineTest }, te
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(1);
   const trace = await parseTrace(testInfo.outputPath('test-results', 'a-passes', 'trace.zip'));
-  expect(trace.actionTree).toContain('attach "screenshot"');
+  expect(trace.apiNames).toContain('_attach "screenshot"');
 });
 
 test('should not throw when screenshot on failure fails', async ({ runInlineTest, server }, testInfo) => {
@@ -983,7 +980,6 @@ test('should record nested steps, even after timeout', async ({ runInlineTest },
     '      page.setContent',
     '  fixture: page',
     '  fixture: context',
-    '  attach "_prompt-0"',
     '  afterAll hook',
     '    fixture: barPage',
     '      barPage setup',
@@ -1043,7 +1039,6 @@ test('should attribute worker fixture teardown to the right test', async ({ runI
   expect(trace2.actionTree).toEqual([
     'Before Hooks',
     'After Hooks',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
     '  fixture: foo',
     '    step in foo teardown',
@@ -1153,7 +1148,6 @@ test('should not corrupt actions when no library trace is present', async ({ run
     'After Hooks',
     '  fixture: foo',
     '    expect.toBe',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
   ]);
 });
@@ -1184,7 +1178,6 @@ test('should record trace for manually created context in a failed test', async
     'page.setContent',
     'expect.toBe',
     'After Hooks',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);
@@ -1272,7 +1265,6 @@ test('should record trace after fixture teardown timeout', {
     'page.evaluate',
     'After Hooks',
     '  fixture: fixture',
-    '  attach "_prompt-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);

tests/playwright-test/ui-mode-trace.spec.ts

@@ -458,7 +458,6 @@ test('attachments tab shows all but top-level .push attachments', async ({ runUI
       - treeitem /step/:
         - group:
           - treeitem /attach \\"foo-attach\\"/
-      - treeitem /attach \\"bar-push\\"/
       - treeitem /attach \\"bar-attach\\"/
   `);
   await page.getByRole('tab', { name: 'Attachments' }).click();

tests/tsconfig.json

@@ -12,6 +12,7 @@
         "baseUrl": "..",
         "paths": {
           "@isomorphic/*": ["packages/playwright-core/src/utils/isomorphic/*"],
+          "@testIsomorphic/*": ["packages/playwright/src/isomorphic/*"],
           "@protocol/*": ["packages/protocol/src/*"],
           "@recorder/*": ["packages/recorder/src/*"],
           "@trace/*": ["packages/trace/src/*"],