chore: do not hide error context (#36000)

packages/html-reporter/src/testResultView.tsx

@@ -165,7 +165,7 @@ function classifyErrors(testErrors: string[], diffs: ImageDiff[], attachments: T
       }
     }
 
-    const context = attachments.find(a => a.name === `_error-context-${i}`);
+    const context = attachments.find(a => a.name === `error-context-${i}`);
     return { type: 'regular', error, context };
   });
 }

packages/playwright/src/errorContext.ts

@@ -32,7 +32,7 @@ export async function attachErrorContext(testInfo: TestInfoImpl, format: 'markdo
       return;
 
     testInfo._attach({
-      name: `_error-context`,
+      name: `error-context`,
       contentType: 'application/json',
       body: Buffer.from(JSON.stringify({
         pageSnapshot: ariaSnapshot,
@@ -63,7 +63,7 @@ export async function attachErrorContext(testInfo: TestInfoImpl, format: 'markdo
 
   for (const [index, error] of errors) {
     const metadata = testInfo.config.metadata as MetadataWithCommitInfo;
-    if (testInfo.attachments.find(a => a.name === `_error-context-${index}`))
+    if (testInfo.attachments.find(a => a.name === `error-context-${index}`))
       continue;
 
     const lines = [
@@ -134,7 +134,7 @@ export async function attachErrorContext(testInfo: TestInfoImpl, format: 'markdo
     await fs.writeFile(filePath, lines.join('\n'), 'utf8');
 
     (testInfo as TestInfoImpl)._attach({
-      name: `_error-context-${index}`,
+      name: `error-context-${index}`,
       contentType: 'text/markdown',
       path: filePath,
     }, undefined);

packages/playwright/src/isomorphic/util.ts

@@ -1,17 +0,0 @@
-/**
- * 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/reporters/base.ts

@@ -337,7 +337,7 @@ export function formatFailure(screen: Screen, config: FullConfig, test: TestCase
     resultLines.push(...errors.map(error => '\n' + error.message));
     for (let i = 0; i < result.attachments.length; ++i) {
       const attachment = result.attachments[i];
-      if (attachment.name.startsWith('_error-context') && attachment.path) {
+      if (attachment.name.startsWith('error-context') && attachment.path) {
         resultLines.push('');
         resultLines.push(screen.colors.dim(`    Error Context: ${relativeFilePath(screen, config, attachment.path)}`));
         continue;

packages/playwright/src/worker/testInfo.ts

@@ -17,7 +17,7 @@
 import fs from 'fs';
 import path from 'path';
 
-import { captureRawStack, monotonicTime, sanitizeForFilePath, stringifyStackFrames, currentZone } from 'playwright-core/lib/utils';
+import { captureRawStack, monotonicTime, sanitizeForFilePath, stringifyStackFrames, currentZone, createGuid } from 'playwright-core/lib/utils';
 
 import { TimeoutManager, TimeoutManagerError, kMaxDeadline } from './timeoutManager';
 import { addSuffixToFilePath, filteredStackTrace, getContainedPath, normalizeAndSaveAttachment, sanitizeFilePathBeforeExtension, trimLongString, windowsFilesystemFriendlyLength } from '../util';
@@ -329,7 +329,7 @@ export class TestInfoImpl implements TestInfo {
       location: data.location,
     };
     this._onStepBegin(payload);
-    this._tracing.appendBeforeActionForStep(stepId, parentStep?.stepId, data.category, data.apiName || data.title, data.params, data.location ? [data.location] : []);
+    this._tracing.appendBeforeActionForStep(stepId, parentStep?.stepId, data.apiName || data.title, data.params, data.location ? [data.location] : []);
     return step;
   }
 
@@ -421,10 +421,13 @@ 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
-      this._tracing.appendTopLevelAttachment(attachment);
+    } else {
+      const callId = `attach@${createGuid()}`;
+      this._tracing.appendBeforeActionForStep(callId, undefined, `Attach "${attachment.name}"`, undefined, []);
+      this._tracing.appendAfterActionForStep(callId, undefined, [attachment]);
+    }
 
     this._onAttach({
       testId: this.testId,

packages/playwright/src/worker/testTracing.ts

@@ -20,7 +20,6 @@ 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';
@@ -268,7 +267,7 @@ export class TestTracing {
     });
   }
 
-  appendBeforeActionForStep(callId: string, parentId: string | undefined, category: string, apiName: string, params: Record<string, any> | undefined, stack: StackFrame[]) {
+  appendBeforeActionForStep(callId: string, parentId: string | undefined, apiName: string, params: Record<string, any> | undefined, stack: StackFrame[]) {
     this._appendTraceEvent({
       type: 'before',
       callId,
@@ -293,16 +292,6 @@ 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

@@ -14,8 +14,6 @@
  * limitations under the License.
  */
 
-import { kTopLevelAttachmentPrefix } from '@testIsomorphic/util';
-
 import type { Language } from '@isomorphic/locatorGenerators';
 import type { ResourceSnapshot } from '@trace/snapshot';
 import type * as trace from '@trace/trace';
@@ -141,7 +139,7 @@ export class MultiTraceModel {
     return this.errors.filter(e => !!e.message).map((error, i) => ({
       stack: error.stack,
       message: error.message,
-      context: this.attachments.find(a => a.name === `_error-context-${i}`),
+      context: this.attachments.find(a => a.name === `error-context-${i}`),
     }));
   }
 }
@@ -351,8 +349,6 @@ 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

@@ -167,7 +167,7 @@ test('should print debug log when failed to connect', async ({ runInlineTest })
   expect(result.exitCode).toBe(1);
   expect(result.failed).toBe(1);
   expect(result.output).toContain('b-debug-log-string');
-  expect(result.results[0].attachments).toEqual([expect.objectContaining({ name: '_error-context-0' })]);
+  expect(result.results[0].attachments).toEqual([expect.objectContaining({ name: 'error-context-0' })]);
 });
 
 test('should record trace', async ({ runInlineTest }) => {
@@ -223,7 +223,7 @@ test('should record trace', async ({ runInlineTest }) => {
     'After Hooks',
     'fixture: page',
     'fixture: context',
-    '_attach "_error-context-0"',
+    'Attach "error-context-0"',
     'Worker Cleanup',
     'fixture: browser',
   ]);

tests/playwright-test/playwright.spec.ts

@@ -516,7 +516,7 @@ test('should work with video: on-first-retry', async ({ runInlineTest }) => {
   const videoFailRetry = fs.readdirSync(dirRetry).find(file => file.endsWith('webm'));
   expect(videoFailRetry).toBeTruthy();
 
-  const errorPrompt = expect.objectContaining({ name: '_error-context-0' });
+  const errorPrompt = expect.objectContaining({ name: 'error-context-0' });
   expect(result.report.suites[0].specs[1].tests[0].results[0].attachments).toEqual([errorPrompt]);
   expect(result.report.suites[0].specs[1].tests[0].results[1].attachments).toEqual([{
     name: 'video',

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

@@ -127,6 +127,7 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => {
     '  fixture: page',
     '  fixture: context',
     '  fixture: request',
+    'Attach "error-context-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);
@@ -728,7 +729,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.apiNames).toContain('_attach "screenshot"');
+  expect(trace.apiNames).toContain('Attach "screenshot"');
 });
 
 test('should not throw when screenshot on failure fails', async ({ runInlineTest, server }, testInfo) => {
@@ -1187,6 +1188,7 @@ test('should not corrupt actions when no library trace is present', async ({ run
     'After Hooks',
     '  fixture: foo',
     '    expect.toBe',
+    'Attach "error-context-0"',
     'Worker Cleanup',
   ]);
 });
@@ -1217,6 +1219,7 @@ test('should record trace for manually created context in a failed test', async
     'page.setContent',
     'expect.toBe',
     'After Hooks',
+    'Attach "error-context-0"',
     'Worker Cleanup',
     '  fixture: browser',
   ]);

tests/playwright-test/reporter-json.spec.ts

@@ -370,7 +370,7 @@ test('attaches error context', async ({ runInlineTest }) => {
     `,
   }, { reporter: 'json' });
 
-  const errorContext = result.report.suites[0].specs[0].tests[0].results[0].attachments.find(a => a.name === '_error-context');
+  const errorContext = result.report.suites[0].specs[0].tests[0].results[0].attachments.find(a => a.name === 'error-context');
   expect(errorContext).toBeDefined();
   expect(errorContext!.contentType).toBe('application/json');
   const json = JSON.parse(Buffer.from(errorContext!.body, 'base64').toString('utf-8'));