chore: trim error context and make it singular (#36117)

This commit is contained in:
Simon Knott 2025-05-28 15:10:24 +02:00 committed by GitHub
parent d1df5893bc
commit 4cb2431c33
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 60 additions and 254 deletions

View File

@ -22,22 +22,6 @@ import { ImageDiffView } from '@web/shared/imageDiffView';
import { TestAttachment } from './types';
import { fixTestInstructions } from '@web/prompts';
export const TestErrorView: React.FC<{
error: string;
testId?: string;
context?: TestAttachment;
}> = ({ error, testId, context }) => {
return (
<CodeSnippet code={error} testId={testId}>
{context && (
<div style={{ position: 'absolute', right: 0, padding: '10px' }}>
<PromptButton context={context} />
</div>
)}
</CodeSnippet>
);
};
export const CodeSnippet = ({ code, children, testId }: React.PropsWithChildren<{ code: string; testId?: string; }>) => {
const html = React.useMemo(() => ansiErrorToHtml(code), [code]);
return (
@ -48,14 +32,15 @@ export const CodeSnippet = ({ code, children, testId }: React.PropsWithChildren<
);
};
const PromptButton: React.FC<{ context: TestAttachment }> = ({ context }) => {
export const PromptButton: React.FC<{ context?: TestAttachment }> = ({ context }) => {
const [copied, setCopied] = React.useState(false);
return <button
className='button'
style={{ minWidth: 100 }}
onClick={async () => {
const text = context.body ? context.body : await fetch(context.path!).then(r => r.text());
await navigator.clipboard.writeText(fixTestInstructions + text);
const contextText = context?.path ? await fetch(context.path!).then(r => r.text()) : context?.body;
const prompt = fixTestInstructions + contextText; // TODO in next PR: enrich with test location, error details and source code.
await navigator.clipboard.writeText(prompt);
setCopied(true);
setTimeout(() => {
setCopied(false);

View File

@ -20,7 +20,7 @@ import { TestFileView } from './testFileView';
import './testFileView.css';
import { msToString } from './utils';
import { AutoChip } from './chip';
import { TestErrorView } from './testErrorView';
import { CodeSnippet } from './testErrorView';
import * as icons from './icons';
import { isMetadataEmpty, MetadataView } from './metadataView';
import { HeaderView } from './headerView';
@ -88,7 +88,7 @@ export const TestFilesHeader: React.FC<{
</div>}
{metadataVisible && <MetadataView metadata={report.metadata}/>}
{!!report.errors.length && <AutoChip header='Errors' dataTestId='report-errors'>
{report.errors.map((error, index) => <TestErrorView key={'test-report-error-message-' + index} error={error}></TestErrorView>)}
{report.errors.map((error, index) => <CodeSnippet key={'test-report-error-message-' + index} code={error}/>)}
</AutoChip>}
</>;
};

View File

@ -24,7 +24,7 @@ import { Anchor, AttachmentLink, generateTraceUrl, testResultHref } from './link
import { statusIcon } from './statusIcon';
import type { ImageDiff } from '@web/shared/imageDiffView';
import { ImageDiffView } from '@web/shared/imageDiffView';
import { CodeSnippet, TestErrorView, TestScreenshotErrorView } from './testErrorView';
import { CodeSnippet, PromptButton, TestScreenshotErrorView } from './testErrorView';
import * as icons from './icons';
import './testResultView.css';
@ -71,26 +71,30 @@ export const TestResultView: React.FC<{
test: TestCase,
result: TestResult,
}> = ({ test, result }) => {
const { screenshots, videos, traces, otherAttachments, diffs, errors, otherAttachmentAnchors, screenshotAnchors } = React.useMemo(() => {
const { screenshots, videos, traces, otherAttachments, diffs, errors, otherAttachmentAnchors, screenshotAnchors, errorContext } = React.useMemo(() => {
const attachments = result.attachments.filter(a => !a.name.startsWith('_'));
const screenshots = new Set(attachments.filter(a => a.contentType.startsWith('image/')));
const screenshotAnchors = [...screenshots].map(a => `attachment-${attachments.indexOf(a)}`);
const videos = attachments.filter(a => a.contentType.startsWith('video/'));
const traces = attachments.filter(a => a.name === 'trace');
const errorContext = attachments.find(a => a.name === 'error-context');
const otherAttachments = new Set<TestAttachment>(attachments);
[...screenshots, ...videos, ...traces].forEach(a => otherAttachments.delete(a));
const otherAttachmentAnchors = [...otherAttachments].map(a => `attachment-${attachments.indexOf(a)}`);
const diffs = groupImageDiffs(screenshots, result);
const errors = classifyErrors(result.errors, diffs, result.attachments);
return { screenshots: [...screenshots], videos, traces, otherAttachments, diffs, errors, otherAttachmentAnchors, screenshotAnchors };
const errors = classifyErrors(result.errors, diffs);
return { screenshots: [...screenshots], videos, traces, otherAttachments, diffs, errors, otherAttachmentAnchors, screenshotAnchors, errorContext };
}, [result]);
return <div className='test-result'>
{!!errors.length && <AutoChip header='Errors'>
<div style={{ position: 'absolute', right: '16px', padding: '10px', zIndex: 1 }}>
<PromptButton context={errorContext} />
</div>
{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!} context={error.context}></TestErrorView>;
return <CodeSnippet key={'test-result-error-message-' + index} code={error.error!}/>;
})}
</AutoChip>}
{!!result.steps.length && <AutoChip header='Test Steps'>
@ -144,8 +148,8 @@ export const TestResultView: React.FC<{
</div>;
};
function classifyErrors(testErrors: string[], diffs: ImageDiff[], attachments: TestAttachment[]) {
return testErrors.map((error, i) => {
function classifyErrors(testErrors: string[], diffs: ImageDiff[]) {
return testErrors.map(error => {
const firstLine = error.split('\n')[0];
if (firstLine.includes('toHaveScreenshot') || firstLine.includes('toMatchSnapshot')) {
const matchingDiff = diffs.find(diff => {
@ -165,8 +169,7 @@ function classifyErrors(testErrors: string[], diffs: ImageDiff[], attachments: T
}
}
const context = attachments.find(a => a.name === `error-context-${i}`);
return { type: 'regular', error, context };
return { type: 'regular', error };
});
}

View File

@ -1,158 +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.
*/
import * as fs from 'fs/promises';
import * as path from 'path';
import { parseErrorStack } from 'playwright-core/lib/utils';
import { stripAnsiEscapes } from './util';
import { codeFrameColumns } from './transform/babelBundle';
import type { MetadataWithCommitInfo } from './isomorphic/types';
import type { TestInfoImpl } from './worker/testInfo';
import type { Location } from '../types/test';
export async function attachErrorContext(testInfo: TestInfoImpl, sourceCache: Map<string, string>, ariaSnapshot: string | undefined) {
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()) {
if (error.message?.includes(singleLineError))
meaningfulSingleLineErrors.delete(singleLineError);
}
}
const errors = [...testInfo.errors.entries()].filter(([, error]) => {
if (!error.message)
return false;
// Skip errors that are just a single line - they are likely to already be the error message.
if (!error.message.includes('\n') && !meaningfulSingleLineErrors.has(error.message))
return false;
return true;
});
for (const [index, error] of errors) {
const metadata = testInfo.config.metadata as MetadataWithCommitInfo;
if (testInfo.attachments.find(a => a.name === `error-context-${index}`))
continue;
const lines = [
`# Test info`,
'',
`- Name: ${testInfo.titlePath.slice(1).join(' >> ')}`,
`- Location: ${testInfo.file}:${testInfo.line}:${testInfo.column}`,
'',
'# Error details',
'',
'```',
stripAnsiEscapes(error.stack || error.message || ''),
'```',
];
if (ariaSnapshot) {
lines.push(
'',
'# Page snapshot',
'',
'```yaml',
ariaSnapshot,
'```',
);
}
const parsedError = error.stack ? parseErrorStack(error.stack, path.sep) : undefined;
const inlineMessage = stripAnsiEscapes(parsedError?.message || error.message || '').split('\n')[0];
const loadedSource = await loadSource(parsedError?.location, testInfo, sourceCache);
if (loadedSource) {
const codeFrame = codeFrameColumns(
loadedSource.source,
{
start: {
line: loadedSource.location.line,
column: loadedSource.location.column
},
},
{
highlightCode: false,
linesAbove: 100,
linesBelow: 100,
message: inlineMessage || undefined,
}
);
lines.push(
'',
'# Test source',
'',
'```ts',
codeFrame,
'```',
);
}
if (metadata.gitDiff) {
lines.push(
'',
'# Local changes',
'',
'```diff',
metadata.gitDiff,
'```',
);
}
const filePath = testInfo.outputPath(errors.length === 1 ? `error-context.md` : `error-context-${index}.md`);
await fs.writeFile(filePath, lines.join('\n'), 'utf8');
(testInfo as TestInfoImpl)._attach({
name: `error-context-${index}`,
contentType: 'text/markdown',
path: filePath,
}, undefined);
}
}
async function loadSource(
errorLocation: Location | undefined,
testLocation: Location,
sourceCache: Map<string, string>
): Promise<{ location: Location, source: string } | undefined> {
if (errorLocation) {
const source = await loadSourceCached(errorLocation.file, sourceCache);
if (source)
return { location: errorLocation, source };
}
// If the error location is not available on the disk (e.g. fake page.evaluate in-browser error), then fallback to the test file.
const source = await loadSourceCached(testLocation.file, sourceCache);
if (source)
return { location: testLocation, source };
return undefined;
}
async function loadSourceCached(file: string, sourceCache: Map<string, string>): Promise<string | undefined> {
let source = sourceCache.get(file);
if (!source) {
try {
// A mild race is Ok here.
source = await fs.readFile(file, 'utf8');
sourceCache.set(file, source);
} catch (e) {
// Ignore errors.
}
}
return source;
}

View File

@ -22,7 +22,6 @@ import { setBoxedStackPrefixes, createGuid, currentZone, debugMode, jsonStringif
import { currentTestInfo } from './common/globals';
import { rootTestType } from './common/testType';
import { attachErrorContext } from './errorContext';
import { stepTitle } from './util';
import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, VideoMode } from '../types/test';
@ -720,8 +719,23 @@ class ArtifactsRecorder {
if (context)
await this._takePageSnapshot(context);
if (!process.env.PLAYWRIGHT_NO_COPY_PROMPT)
await attachErrorContext(this._testInfo, this._sourceCache, this._pageSnapshot);
if (this._pageSnapshot && this._testInfo.errors.length > 0 && !this._testInfo.attachments.some(a => a.name === 'error-context')) {
const lines = [
'# Page snapshot',
'',
'```yaml',
this._pageSnapshot,
'```',
];
const filePath = this._testInfo.outputPath('error-context.md');
await fs.promises.writeFile(filePath, lines.join('\n'), 'utf8');
this._testInfo._attach({
name: 'error-context',
contentType: 'text/markdown',
path: filePath,
}, undefined);
}
}
private async _startTraceChunkOnContextCreation(tracing: Tracing) {

View File

@ -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 === 'error-context' && attachment.path) {
resultLines.push('');
resultLines.push(screen.colors.dim(` Error Context: ${relativeFilePath(screen, config, attachment.path)}`));
continue;

View File

@ -61,13 +61,6 @@ function ErrorView({ message, error, sdkLanguage, revealInSource }: { message: s
longLocation = stackFrame.file + ':' + stackFrame.line;
}
const prompt = useAsyncMemo(async () => {
if (!error.context)
return;
const response = await fetch(attachmentURL(error.context));
return fixTestInstructions + await response.text();
}, [error], undefined);
return <div style={{ display: 'flex', flexDirection: 'column', overflowX: 'clip' }}>
<div className='hbox' style={{
alignItems: 'center',
@ -81,9 +74,6 @@ function ErrorView({ message, error, sdkLanguage, revealInSource }: { message: s
{location && <div className='action-location'>
@ <span title={longLocation} onClick={() => revealInSource(error)}>{location}</span>
</div>}
<span style={{ position: 'absolute', right: '5px' }}>
{prompt && <CopyPromptButton prompt={prompt} />}
</span>
</div>
<ErrorMessage error={message} />
@ -92,14 +82,27 @@ function ErrorView({ message, error, sdkLanguage, revealInSource }: { message: s
export const ErrorsTab: React.FunctionComponent<{
errorsModel: ErrorsTabModel,
model?: modelUtil.MultiTraceModel,
wallTime: number,
sdkLanguage: Language,
revealInSource: (error: modelUtil.ErrorDescription) => void,
}> = ({ errorsModel, sdkLanguage, revealInSource, wallTime }) => {
}> = ({ errorsModel, model, sdkLanguage, revealInSource, wallTime }) => {
const errorContext = useAsyncMemo(async () => {
const attachment = model?.attachments.find(a => a.name === 'error-context');
if (!attachment)
return;
return await fetch(attachmentURL(attachment)).then(r => r.text());
}, [model], undefined);
const prompt = fixTestInstructions + (errorContext ?? ''); // TODO in next PR: enrich with test location, error details and source code, similar to errorContext.ts
if (!errorsModel.errors.size)
return <PlaceholderPanel text='No errors' />;
return <div className='fill' style={{ overflow: 'auto' }}>
<span style={{ position: 'absolute', right: '5px', top: '5px', zIndex: 1 }}>
{prompt && <CopyPromptButton prompt={prompt} />}
</span>
{[...errorsModel.errors.entries()].map(([message, error]) => {
const errorId = `error-${wallTime}-${message}`;
return <ErrorView key={errorId} message={message} error={error} revealInSource={revealInSource} sdkLanguage={sdkLanguage} />;

View File

@ -54,7 +54,6 @@ export type ErrorDescription = {
action?: ActionTraceEventInContext;
stack?: StackFrame[];
message: string;
context?: trace.AfterActionTraceEventAttachment & { traceUrl: string };
};
export type Attachment = trace.AfterActionTraceEventAttachment & { traceUrl: string };
@ -139,7 +138,6 @@ 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}`),
}));
}
}

View File

@ -190,7 +190,7 @@ export const Workbench: React.FunctionComponent<{
id: 'errors',
title: 'Errors',
errorCount: errorsModel.errors.size,
render: () => <ErrorsTab errorsModel={errorsModel} sdkLanguage={sdkLanguage} revealInSource={error => {
render: () => <ErrorsTab errorsModel={errorsModel} model={model} sdkLanguage={sdkLanguage} revealInSource={error => {
if (error.action)
setSelectedAction(error.action);
else

View File

@ -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([]);
});
test('should record trace', async ({ runInlineTest }) => {
@ -223,7 +223,6 @@ test('should record trace', async ({ runInlineTest }) => {
'After Hooks',
'Fixture "page"',
'Fixture "context"',
'Attach "error-context-0"',
'Worker Cleanup',
'Fixture "browser"',
]);

View File

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

View File

@ -127,7 +127,6 @@ test('should record api trace', async ({ runInlineTest, server }, testInfo) => {
' Fixture "page"',
' Fixture "context"',
' Fixture "request"',
'Attach "error-context-0"',
'Worker Cleanup',
' Fixture "browser"',
]);
@ -992,6 +991,7 @@ test('should record nested steps, even after timeout', async ({ runInlineTest },
' Expect "barPage teardown"',
' Step "step in barPage teardown"',
' Close context',
'Attach "error-context"',
'Worker Cleanup',
' Fixture "browser"',
]);
@ -1188,7 +1188,6 @@ 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',
]);
});
@ -1219,7 +1218,6 @@ test('should record trace for manually created context in a failed test', async
'Set content',
'Expect "toBe"',
'After Hooks',
'Attach "error-context-0"',
'Worker Cleanup',
' Fixture "browser"',
]);

View File

@ -2874,7 +2874,7 @@ for (const useIntermediateMergeReport of [true, false] as const) {
await expect(page.locator('.tree-item', { hasText: 'stdout' })).toHaveCount(1);
});
test('should include diff in AI prompt', async ({ runInlineTest, writeFiles, showReport, page }) => {
test.fixme('should include diff in AI prompt', async ({ runInlineTest, writeFiles, showReport, page }) => {
const files = {
'uncommitted.txt': `uncommitted file`,
'playwright.config.ts': `export default {}`,
@ -2919,22 +2919,6 @@ for (const useIntermediateMergeReport of [true, false] as const) {
expect(prompt, 'contains diff').toContain(`+ expect(2).toBe(3);`);
});
test('should not show prompt for empty timeout error', async ({ runInlineTest, showReport, page }) => {
const result = await runInlineTest({
'example.spec.ts': `
import { test, expect } from '@playwright/test';
test('sample', async ({ page }) => {
test.setTimeout(2000);
await page.setChecked('input', true);
});
`,
}, { reporter: 'dot,html' }, { PLAYWRIGHT_HTML_OPEN: 'never' });
expect(result.exitCode).toBe(1);
await showReport();
await page.getByRole('link', { name: 'sample' }).click();
await expect(page.getByRole('button', { name: 'Copy prompt' })).toHaveCount(1);
});
test('should include snapshot when page wasnt closed', async ({ runInlineTest, showReport, page }) => {
const result = await runInlineTest({
'example.spec.ts': `

View File

@ -15,7 +15,6 @@
*/
import path from 'path';
import fs from 'fs';
import { test, expect } from './playwright-test-fixtures';
for (const useIntermediateMergeReport of [false, true] as const) {
@ -195,7 +194,8 @@ for (const useIntermediateMergeReport of [false, true] as const) {
const result = await runInlineTest({
'a.test.js': `
const { test, expect } = require('@playwright/test');
test('one', async ({}) => {
test('one', async ({ page }) => {
await page.setContent('<div>hello</div>');
expect(1).toBe(0);
});
`,
@ -207,26 +207,5 @@ for (const useIntermediateMergeReport of [false, true] as const) {
expect(text).toContain(`Error Context: ${path.join('test-results', 'a-one', 'error-context.md')}`);
expect(result.exitCode).toBe(1);
});
test('should show error context if exception contains non-existent file', async ({ runInlineTest, useIntermediateMergeReport }) => {
const result = await runInlineTest({
'a.test.js': `
const { test, expect } = require('@playwright/test');
test('one', async ({ page }) => {
await page.evaluate(() => {
throw new Error('error');
});
});
`,
}, { reporter: 'line' });
if (useIntermediateMergeReport)
expect(result.output).toContain(`Error Context: ${path.join('blob-report', 'resources')}`);
else
expect(result.output).toContain(`Error Context: ${path.join('test-results', 'a-one', 'error-context.md')}`);
const file = /Error Context: (.*)/.exec(result.output)?.[1];
const content = await fs.promises.readFile(path.join(result.report.config.rootDir, file), 'utf8');
expect(content).toContain('^ Error: page.evaluate: Error: error');
expect(result.exitCode).toBe(1);
});
});
}

View File

@ -73,7 +73,7 @@ test('should merge web assertion events', async ({ runUITest }, testInfo) => {
]);
});
test('should merge screenshot assertions', async ({ runUITest }, testInfo) => {
test('should merge screenshot assertions', async ({ runUITest }) => {
const { page } = await runUITest({
'a.test.ts': `
import { test, expect } from '@playwright/test';
@ -95,6 +95,7 @@ test('should merge screenshot assertions', async ({ runUITest }, testInfo) => {
/Set content/,
/Expect "toHaveScreenshot"[\d.]+m?s/,
/After Hooks[\d.]+m?s/,
/Attach "error-context"/,
/Worker Cleanup[\d.]+m?s/,
]);
});
@ -554,7 +555,7 @@ test('skipped steps should have an indicator', async ({ runUITest }) => {
await expect(skippedMarker).toHaveAccessibleName('skipped');
});
test('should show copy prompt button in errors tab', async ({ runUITest }) => {
test.fixme('should show copy prompt button in errors tab', async ({ runUITest }) => {
const { page } = await runUITest({
'a.spec.ts': `
import { test, expect } from '@playwright/test';