chore: refactor error context (#35613)

packages/html-reporter/src/testErrorView.tsx

@@ -20,17 +20,18 @@ import './testErrorView.css';
 import type { ImageDiff } from '@web/shared/imageDiffView';
 import { ImageDiffView } from '@web/shared/imageDiffView';
 import { TestAttachment } from './types';
+import { fixTestInstructions } from '@web/prompts';
 
 export const TestErrorView: React.FC<{
   error: string;
   testId?: string;
-  prompt?: TestAttachment;
+  context?: TestAttachment;
-}> = ({ error, testId, prompt }) => {
+}> = ({ error, testId, context }) => {
   return (
     <CodeSnippet code={error} testId={testId}>
-      {prompt && (
+      {context && (
         <div style={{ position: 'absolute', right: 0, padding: '10px' }}>
-          <PromptButton prompt={prompt} />
+          <PromptButton context={context} />
         </div>
       )}
     </CodeSnippet>
@@ -47,14 +48,14 @@ export const CodeSnippet = ({ code, children, testId }: React.PropsWithChildren<
   );
 };
 
-const PromptButton: React.FC<{ prompt: TestAttachment }> = ({ prompt }) => {
+const PromptButton: React.FC<{ context: TestAttachment }> = ({ context }) => {
   const [copied, setCopied] = React.useState(false);
   return <button
     className='button'
     style={{ minWidth: 100 }}
     onClick={async () => {
-      const text = prompt.body ? prompt.body : await fetch(prompt.path!).then(r => r.text());
+      const text = context.body ? context.body : await fetch(context.path!).then(r => r.text());
-      await navigator.clipboard.writeText(text);
+      await navigator.clipboard.writeText(fixTestInstructions + text);
       setCopied(true);
       setTimeout(() => {
         setCopied(false);

packages/html-reporter/src/testResultView.tsx

@@ -90,7 +90,7 @@ export const TestResultView: React.FC<{
       {errors.map((error, index) => {
         if (error.type === 'screenshot')
           return <TestScreenshotErrorView key={'test-result-error-message-' + index} errorPrefix={error.errorPrefix} diff={error.diff!} errorSuffix={error.errorSuffix}></TestScreenshotErrorView>;
-        return <TestErrorView key={'test-result-error-message-' + index} error={error.error!} prompt={error.prompt}></TestErrorView>;
+        return <TestErrorView key={'test-result-error-message-' + index} error={error.error!} context={error.context}></TestErrorView>;
       })}
     </AutoChip>}
     {!!result.steps.length && <AutoChip header='Test Steps'>
@@ -165,8 +165,8 @@ function classifyErrors(testErrors: string[], diffs: ImageDiff[], attachments: T
       }
     }
 
-    const prompt = attachments.find(a => a.name === `_prompt-${i}`);
+    const context = attachments.find(a => a.name === `_error-context-${i}`);
-    return { type: 'regular', error, prompt };
+    return { type: 'regular', error, context };
   });
 }
 

packages/playwright/src/DEPS.list

@@ -14,5 +14,5 @@ common/
 [internalsForTest.ts]
 **
 
-[prompt.ts]
+[errorContext.ts]
 ./transform/babelBundle.ts

packages/playwright/src/errorContext.ts

@@ -22,14 +22,25 @@ import { parseErrorStack } from 'playwright-core/lib/utils';
 import { stripAnsiEscapes } from './util';
 import { codeFrameColumns } from './transform/babelBundle';
 
-import type { TestInfo } from '../types/test';
 import type { MetadataWithCommitInfo } from './isomorphic/types';
 import type { TestInfoImpl } from './worker/testInfo';
 
-export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<string, string>, ariaSnapshot: string | undefined) {
+export async function attachErrorContext(testInfo: TestInfoImpl, format: 'markdown' | 'json', sourceCache: Map<string, string>, ariaSnapshot: string | undefined) {
-  if (process.env.PLAYWRIGHT_NO_COPY_PROMPT)
+  if (format === 'json') {
+    if (!ariaSnapshot)
       return;
 
+    testInfo._attach({
+      name: `_error-context`,
+      contentType: 'application/json',
+      body: Buffer.from(JSON.stringify({
+        pageSnapshot: ariaSnapshot,
+      })),
+    }, undefined);
+
+    return;
+  }
+
   const meaningfulSingleLineErrors = new Set(testInfo.errors.filter(e => e.message && !e.message.includes('\n')).map(e => e.message!));
   for (const error of testInfo.errors) {
     for (const singleLineError of meaningfulSingleLineErrors.keys()) {
@@ -51,16 +62,10 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
 
   for (const [index, error] of errors) {
     const metadata = testInfo.config.metadata as MetadataWithCommitInfo;
-    if (testInfo.attachments.find(a => a.name === `_prompt-${index}`))
+    if (testInfo.attachments.find(a => a.name === `_error-context-${index}`))
       continue;
 
-    const promptParts = [
+    const lines = [
-      `# Instructions`,
-      '',
-      `- Following Playwright test failed.`,
-      `- Explain why, be concise, respect Playwright best practices.`,
-      `- Provide a snippet of code with the fix, if possible.`,
-      '',
       `# Test info`,
       '',
       `- Name: ${testInfo.titlePath.slice(1).join(' >> ')}`,
@@ -74,7 +79,7 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
     ];
 
     if (ariaSnapshot) {
-      promptParts.push(
+      lines.push(
           '',
           '# Page snapshot',
           '',
@@ -103,7 +108,7 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
           message: inlineMessage || undefined,
         }
     );
-    promptParts.push(
+    lines.push(
         '',
         '# Test source',
         '',
@@ -113,7 +118,7 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
     );
 
     if (metadata.gitDiff) {
-      promptParts.push(
+      lines.push(
           '',
           '# Local changes',
           '',
@@ -123,30 +128,17 @@ export async function attachErrorPrompts(testInfo: TestInfo, sourceCache: Map<st
       );
     }
 
-    const promptPath = testInfo.outputPath(errors.length === 1 ? `prompt.md` : `prompt-${index}.md`);
+    const filePath = testInfo.outputPath(errors.length === 1 ? `error-context.md` : `error-context-${index}.md`);
-    await fs.writeFile(promptPath, promptParts.join('\n'), 'utf8');
+    await fs.writeFile(filePath, lines.join('\n'), 'utf8');
 
     (testInfo as TestInfoImpl)._attach({
-      name: `_prompt-${index}`,
+      name: `_error-context-${index}`,
       contentType: 'text/markdown',
-      path: promptPath,
+      path: filePath,
     }, undefined);
   }
 }
 
-export async function attachErrorContext(testInfo: TestInfo, ariaSnapshot: string | undefined) {
-  if (!ariaSnapshot)
-    return;
-
-  (testInfo as TestInfoImpl)._attach({
-    name: `_error-context`,
-    contentType: 'application/json',
-    body: Buffer.from(JSON.stringify({
-      pageSnapshot: ariaSnapshot,
-    })),
-  }, undefined);
-}
-
 async function loadSource(file: string, sourceCache: Map<string, string>) {
   let source = sourceCache.get(file);
   if (!source) {

packages/playwright/src/index.ts

@@ -22,7 +22,7 @@ import { setBoxedStackPrefixes, asLocator, createGuid, currentZone, debugMode, i
 
 import { currentTestInfo } from './common/globals';
 import { rootTestType } from './common/testType';
-import { attachErrorContext, attachErrorPrompts } from './prompt';
+import { attachErrorContext } from './errorContext';
 
 import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, VideoMode } from '../types/test';
 import type { ContextReuseMode } from './common/config';
@@ -55,13 +55,15 @@ type TestFixtures = PlaywrightTestArgs & PlaywrightTestOptions & {
   _contextFactory: (options?: BrowserContextOptions) => Promise<BrowserContext>;
 };
 
+type ErrorContextOption = { format: 'json' | 'markdown' } | undefined;
+
 type WorkerFixtures = PlaywrightWorkerArgs & PlaywrightWorkerOptions & {
   playwright: PlaywrightImpl;
   _browserOptions: LaunchOptions;
   _optionContextReuseMode: ContextReuseMode,
   _optionConnectOptions: PlaywrightWorkerOptions['connectOptions'],
   _reuseContext: boolean,
-  _optionAttachErrorContext: boolean,
+  _optionErrorContext: ErrorContextOption,
 };
 
 const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
@@ -245,13 +247,13 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
     playwright._defaultContextNavigationTimeout = undefined;
   }, { auto: 'all-hooks-included',  title: 'context configuration', box: true } as any],
 
-  _setupArtifacts: [async ({ playwright, screenshot, _optionAttachErrorContext }, use, testInfo) => {
+  _setupArtifacts: [async ({ playwright, screenshot, _optionErrorContext }, use, testInfo) => {
     // This fixture has a separate zero-timeout slot to ensure that artifact collection
     // happens even after some fixtures or hooks time out.
     // Now that default test timeout is known, we can replace zero with an actual value.
     testInfo.setTimeout(testInfo.project.timeout);
 
-    const artifactsRecorder = new ArtifactsRecorder(playwright, tracing().artifactsDir(), screenshot, _optionAttachErrorContext);
+    const artifactsRecorder = new ArtifactsRecorder(playwright, tracing().artifactsDir(), screenshot, _optionErrorContext);
     await artifactsRecorder.willStartTest(testInfo as TestInfoImpl);
 
     const tracingGroupSteps: TestStepInternal[] = [];
@@ -393,7 +395,7 @@ const playwrightFixtures: Fixtures<TestFixtures, WorkerFixtures> = ({
 
   _optionContextReuseMode: ['none', { scope: 'worker', option: true }],
   _optionConnectOptions: [undefined, { scope: 'worker', option: true }],
-  _optionAttachErrorContext: [false, { scope: 'worker', option: true }],
+  _optionErrorContext: [process.env.PLAYWRIGHT_NO_COPY_PROMPT ? undefined : { format: 'markdown' }, { scope: 'worker', option: true }],
 
   _reuseContext: [async ({ video, _optionContextReuseMode }, use) => {
     let mode = _optionContextReuseMode;
@@ -622,12 +624,12 @@ class ArtifactsRecorder {
   private _screenshotRecorder: SnapshotRecorder;
   private _pageSnapshot: string | undefined;
   private _sourceCache: Map<string, string> = new Map();
-  private _attachErrorContext: boolean;
+  private _errorContext: ErrorContextOption;
 
-  constructor(playwright: PlaywrightImpl, artifactsDir: string, screenshot: ScreenshotOption, attachErrorContext: boolean) {
+  constructor(playwright: PlaywrightImpl, artifactsDir: string, screenshot: ScreenshotOption, errorContext: ErrorContextOption) {
     this._playwright = playwright;
     this._artifactsDir = artifactsDir;
-    this._attachErrorContext = attachErrorContext;
+    this._errorContext = errorContext;
     const screenshotOptions = typeof screenshot === 'string' ? undefined : screenshot;
     this._startedCollectingArtifacts = Symbol('startedCollectingArtifacts');
 
@@ -671,7 +673,7 @@ class ArtifactsRecorder {
   }
 
   private async _takePageSnapshot(context: BrowserContext) {
-    if (process.env.PLAYWRIGHT_NO_COPY_PROMPT)
+    if (!this._errorContext)
       return;
     if (this._testInfo.errors.length === 0)
       return;
@@ -719,10 +721,8 @@ class ArtifactsRecorder {
     if (context)
       await this._takePageSnapshot(context);
 
-    if (this._attachErrorContext)
+    if (this._errorContext)
-      await attachErrorContext(this._testInfo, this._pageSnapshot);
+      await attachErrorContext(this._testInfo, this._errorContext.format, this._sourceCache, this._pageSnapshot);
-    else
-      await attachErrorPrompts(this._testInfo, this._sourceCache, this._pageSnapshot);
   }
 
   private async _startTraceChunkOnContextCreation(tracing: Tracing) {

packages/playwright/src/isomorphic/testServerInterface.ts

@@ -102,7 +102,7 @@ export interface TestServerInterface {
     projects?: string[];
     reuseContext?: boolean;
     connectWsEndpoint?: string;
-    attachErrorContext?: boolean;
+    errorContext?: { format: 'json' | 'markdown' };
   }): Promise<{
     status: reporterTypes.FullResult['status'];
   }>;

packages/playwright/src/reporters/base.ts

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

packages/playwright/src/runner/testServer.ts

@@ -314,7 +314,7 @@ export class TestServerDispatcher implements TestServerInterface {
         ...(params.headed !== undefined ? { headless: !params.headed } : {}),
         _optionContextReuseMode: params.reuseContext ? 'when-possible' : undefined,
         _optionConnectOptions: params.connectWsEndpoint ? { wsEndpoint: params.connectWsEndpoint } : undefined,
-        _optionAttachErrorContext: params.attachErrorContext,
+        _optionErrorContext: params.errorContext,
       },
       ...(params.updateSnapshots ? { updateSnapshots: params.updateSnapshots } : {}),
       ...(params.updateSourceMethod ? { updateSourceMethod: params.updateSourceMethod } : {}),

packages/trace-viewer/src/ui/errorsTab.tsx

@@ -26,6 +26,7 @@ import { ToolbarButton } from '@web/components/toolbarButton';
 import { useIsLLMAvailable, useLLMChat } from './llm';
 import { useAsyncMemo } from '@web/uiUtils';
 import { attachmentURL } from './attachmentsTab';
+import { fixTestInstructions } from '@web/prompts';
 
 const CopyPromptButton: React.FC<{ prompt: string }> = ({ prompt }) => {
   return (
@@ -67,10 +68,10 @@ function Error({ message, error, errorId, sdkLanguage, revealInSource }: { messa
   }
 
   const prompt = useAsyncMemo(async () => {
-    if (!error.prompt)
+    if (!error.context)
       return;
-    const response = await fetch(attachmentURL(error.prompt));
+    const response = await fetch(attachmentURL(error.context));
-    return await response.text();
+    return fixTestInstructions + await response.text();
   }, [error], undefined);
 
   return <div style={{ display: 'flex', flexDirection: 'column', overflowX: 'clip' }}>

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

@@ -56,7 +56,7 @@ export type ErrorDescription = {
   action?: ActionTraceEventInContext;
   stack?: StackFrame[];
   message: string;
-  prompt?: trace.AfterActionTraceEventAttachment & { traceUrl: string };
+  context?: trace.AfterActionTraceEventAttachment & { traceUrl: string };
 };
 
 export type Attachment = trace.AfterActionTraceEventAttachment & { traceUrl: string };
@@ -141,7 +141,7 @@ export class MultiTraceModel {
     return this.errors.filter(e => !!e.message).map((error, i) => ({
       stack: error.stack,
       message: error.message,
-      prompt: this.attachments.find(a => a.name === `_prompt-${i}`),
+      context: this.attachments.find(a => a.name === `_error-context-${i}`),
     }));
   }
 }

packages/web/src/prompts.ts

@@ -0,0 +1,23 @@
+/**
+ * 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 fixTestInstructions = `
+# Instructions
+
+- Following Playwright test failed.
+- Explain why, be concise, respect Playwright best practices.
+- Provide a snippet of code with the fix, if possible.
+`.trimStart();

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: '_prompt-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 "_prompt-0"',
+    '_attach "_error-context-0"',
     'Worker Cleanup',
     'fixture: browser',
   ]);

tests/playwright-test/playwright.spec.ts

@@ -510,13 +510,13 @@ test('should work with video: on-first-retry', async ({ runInlineTest }) => {
   expect(fs.existsSync(dirPass)).toBeFalsy();
 
   const dirFail = test.info().outputPath('test-results', 'a-fail-chromium');
-  expect(fs.readdirSync(dirFail)).toEqual(['prompt.md']);
+  expect(fs.readdirSync(dirFail)).toEqual(['error-context.md']);
 
   const dirRetry = test.info().outputPath('test-results', 'a-fail-chromium-retry1');
   const videoFailRetry = fs.readdirSync(dirRetry).find(file => file.endsWith('webm'));
   expect(videoFailRetry).toBeTruthy();
 
-  const errorPrompt = expect.objectContaining({ name: '_prompt-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/reporter-json.spec.ts

@@ -359,7 +359,7 @@ test('should report parallelIndex', async ({ runInlineTest }, testInfo) => {
 test('attaches error context', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'playwright.config.ts': `
-          export default { use: { _optionAttachErrorContext: true } };
+          export default { use: { _optionErrorContext: { format: 'json' } } };
     `,
     'a.test.js': `
       const { test, expect } = require('@playwright/test');

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

@@ -190,7 +190,7 @@ for (const useIntermediateMergeReport of [false, true] as const) {
       expect(result.exitCode).toBe(1);
     });
 
-    test('should show error prompt with relative path', async ({ runInlineTest, useIntermediateMergeReport }) => {
+    test('should show error context with relative path', async ({ runInlineTest, useIntermediateMergeReport }) => {
       const result = await runInlineTest({
         'a.test.js': `
           const { test, expect } = require('@playwright/test');
@@ -201,9 +201,9 @@ for (const useIntermediateMergeReport of [false, true] as const) {
       }, { reporter: 'line' });
       const text = result.output;
       if (useIntermediateMergeReport)
-        expect(text).toContain(`Error Prompt: ${path.join('blob-report', 'resources')}`);
+        expect(text).toContain(`Error Context: ${path.join('blob-report', 'resources')}`);
       else
-        expect(text).toContain(`Error Prompt: ${path.join('test-results', 'a-one', 'prompt.md')}`);
+        expect(text).toContain(`Error Context: ${path.join('test-results', 'a-one', 'error-context.md')}`);
       expect(result.exitCode).toBe(1);
     });
   });