diff --git a/packages/playwright/src/worker/testTracing.ts b/packages/playwright/src/worker/testTracing.ts index 566f54e271..89a6e99d44 100644 --- a/packages/playwright/src/worker/testTracing.ts +++ b/packages/playwright/src/worker/testTracing.ts @@ -21,7 +21,8 @@ import fs from 'fs'; import path from 'path'; import { ManualPromise, calculateSha1, monotonicTime } from 'playwright-core/lib/utils'; import { yauzl, yazl } from 'playwright-core/lib/zipBundle'; -import type { TestInfo } from '../../types/test'; +import type { TestInfo, TestInfoError } from '../../types/test'; +import { filteredStackTrace } from '../util'; export type Attachment = TestInfo['attachments'][0]; @@ -101,6 +102,16 @@ export class TestTracing { }); } + appendForError(error: TestInfoError) { + const rawStack = error.stack?.split('\n') || []; + const stack = rawStack ? filteredStackTrace(rawStack) : []; + this._appendTraceEvent({ + type: 'error', + message: error.message || String(error.value), + stack, + }); + } + appendStdioToTrace(type: 'stdout' | 'stderr', chunk: string | Buffer) { this._appendTraceEvent({ type, diff --git a/packages/playwright/src/worker/workerMain.ts b/packages/playwright/src/worker/workerMain.ts index 864677ac5c..b3bb77a0bc 100644 --- a/packages/playwright/src/worker/workerMain.ts +++ b/packages/playwright/src/worker/workerMain.ts @@ -22,7 +22,7 @@ import { ConfigLoader } from '../common/configLoader'; import type { Suite, TestCase } from '../common/test'; import type { Annotation, FullConfigInternal, FullProjectInternal } from '../common/config'; import { FixtureRunner } from './fixtureRunner'; -import { ManualPromise, captureLibraryStackTrace, gracefullyCloseAll, removeFolders } from 'playwright-core/lib/utils'; +import { ManualPromise, gracefullyCloseAll, removeFolders } from 'playwright-core/lib/utils'; import { TestInfoImpl } from './testInfo'; import { TimeoutManager, type TimeSlot } from './timeoutManager'; import { ProcessRunner } from '../common/process'; @@ -372,7 +372,7 @@ export class WorkerMain extends ProcessRunner { return; } - const error = await testInfo._runAndFailOnError(async () => { + await testInfo._runAndFailOnError(async () => { // Now run the test itself. debugTest(`test function started`); const fn = test.fn; // Extract a variable to get a better stack trace ("myTest" vs "TestCase.myTest [as fn]"). @@ -380,17 +380,8 @@ export class WorkerMain extends ProcessRunner { debugTest(`test function finished`); }, 'allowSkips'); - // If there are no steps with errors in the test, but the test has an error - append artificial trace entry. - if (error && !testInfo._steps.some(s => !!s.error)) { - const frames = error.stack ? captureLibraryStackTrace(error.stack.split('\n')).frames : []; - const step = testInfo._addStep({ - wallTime: Date.now(), - title: error.message || 'error', - category: 'hook', - location: frames[0], - }); - step.complete({ error }); - } + for (const error of testInfo.errors) + testInfo._tracing.appendForError(error); }); if (didFailBeforeAllForSuite) { diff --git a/packages/trace-viewer/src/entries.ts b/packages/trace-viewer/src/entries.ts index 591fd0628a..1ea9717b53 100644 --- a/packages/trace-viewer/src/entries.ts +++ b/packages/trace-viewer/src/entries.ts @@ -36,6 +36,7 @@ export type ContextEntry = { actions: ActionEntry[]; events: (trace.EventTraceEvent | trace.ConsoleMessageTraceEvent)[]; stdio: trace.StdioTraceEvent[]; + errors: trace.ErrorTraceEvent[]; hasSource: boolean; }; @@ -68,6 +69,7 @@ export function createEmptyContext(): ContextEntry { resources: [], actions: [], events: [], + errors: [], stdio: [], hasSource: false }; diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 0540be1c44..bf7a742cc1 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/packages/trace-viewer/src/traceModel.ts @@ -223,6 +223,10 @@ export class TraceModel { contextEntry!.stdio.push(event); break; } + case 'error': { + contextEntry!.errors.push(event); + break; + } case 'console': { contextEntry!.events.push(event); break; diff --git a/packages/trace-viewer/src/ui/errorsTab.tsx b/packages/trace-viewer/src/ui/errorsTab.tsx index 380b02323a..eb1b529c26 100644 --- a/packages/trace-viewer/src/ui/errorsTab.tsx +++ b/packages/trace-viewer/src/ui/errorsTab.tsx @@ -20,20 +20,50 @@ import type * as modelUtil from './modelUtil'; import { PlaceholderPanel } from './placeholderPanel'; import { renderAction } from './actionList'; import type { Language } from '@isomorphic/locatorGenerators'; +import type { StackFrame } from '@protocol/channels'; + +type ErrorDescription = { + action?: modelUtil.ActionTraceEventInContext; + stack?: StackFrame[]; +}; type ErrorsTabModel = { - errors: Map; + errors: Map; }; +function errorsFromActions(model: modelUtil.MultiTraceModel): Map { + const errors = new Map(); + for (const action of model.actions || []) { + // Overwrite errors with the last one. + if (!action.error?.message || errors.has(action.error.message)) + continue; + errors.set(action.error.message, { + action, + stack: action.stack, + }); + } + return errors; +} + +function errorsFromTestRunner(model: modelUtil.MultiTraceModel): Map { + const actionErrors = errorsFromActions(model); + const errors = new Map(); + for (const error of model.errors || []) { + if (!error.message || errors.has(error.message)) + continue; + errors.set(error.message, actionErrors.get(error.message) || error); + } + return errors; +} + export function useErrorsTabModel(model: modelUtil.MultiTraceModel | undefined): ErrorsTabModel { return React.useMemo(() => { - const errors = new Map(); - for (const action of model?.actions || []) { - // Overwrite errors with the last one. - if (action.error?.message) - errors.set(action.error.message, action); - } - return { errors }; + if (!model) + return { errors: new Map() }; + // Feature detection: if there is test runner info, pick errors from the 'error' trace events. + // If there are no test errors, but there are action errors - render those instead. + const testHasErrors = !!model.errors.length; + return { errors: testHasErrors ? errorsFromTestRunner(model) : errorsFromActions(model) }; }, [model]); } @@ -46,13 +76,14 @@ export const ErrorsTab: React.FunctionComponent<{ return ; return
- {[...errorsModel.errors.entries()].map(([message, action]) => { + {[...errorsModel.errors.entries()].map(([message, error]) => { let location: string | undefined; let longLocation: string | undefined; - if (action.stack?.[0]) { - const file = action.stack[0].file.replace(/.*\/(.*)/, '$1'); - location = file + ':' + action.stack[0].line; - longLocation = action.stack[0].file + ':' + action.stack[0].line; + const stackFrame = error.stack?.[0]; + if (stackFrame) { + const file = stackFrame.file.replace(/.*\/(.*)/, '$1'); + location = file + ':' + stackFrame.line; + longLocation = stackFrame.file + ':' + stackFrame.line; } return
- {renderAction(action, { sdkLanguage })} + {error.action && renderAction(error.action, { sdkLanguage })} {location &&
- @ revealInSource(action)}>{location} + @ error.action && revealInSource(error.action)}>{location}
}
diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index f1379b8e46..f29b4f7616 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -61,6 +61,7 @@ export class MultiTraceModel { readonly actions: ActionTraceEventInContext[]; readonly events: (trace.EventTraceEvent | trace.ConsoleMessageTraceEvent)[]; readonly stdio: trace.StdioTraceEvent[]; + readonly errors: trace.ErrorTraceEvent[]; readonly hasSource: boolean; readonly sdkLanguage: Language | undefined; readonly testIdAttributeName: string | undefined; @@ -86,6 +87,7 @@ export class MultiTraceModel { this.actions = mergeActions(contexts); this.events = ([] as (trace.EventTraceEvent | trace.ConsoleMessageTraceEvent)[]).concat(...contexts.map(c => c.events)); this.stdio = ([] as trace.StdioTraceEvent[]).concat(...contexts.map(c => c.stdio)); + this.errors = ([] as trace.ErrorTraceEvent[]).concat(...contexts.map(c => c.errors)); this.hasSource = contexts.some(c => c.hasSource); this.resources = [...contexts.map(c => c.resources)].flat(); diff --git a/packages/trace/src/trace.ts b/packages/trace/src/trace.ts index 1155da43ec..651c41dc19 100644 --- a/packages/trace/src/trace.ts +++ b/packages/trace/src/trace.ts @@ -145,6 +145,12 @@ export type StdioTraceEvent = { base64?: string; }; +export type ErrorTraceEvent = { + type: 'error'; + message: string; + stack?: StackFrame[]; +}; + export type TraceEvent = ContextCreatedTraceEvent | ScreencastFrameTraceEvent | @@ -157,4 +163,5 @@ export type TraceEvent = ConsoleMessageTraceEvent | ResourceSnapshotTraceEvent | FrameSnapshotTraceEvent | - StdioTraceEvent; + StdioTraceEvent | + ErrorTraceEvent; diff --git a/tests/config/utils.ts b/tests/config/utils.ts index 53e177f955..88cff686b3 100644 --- a/tests/config/utils.ts +++ b/tests/config/utils.ts @@ -156,7 +156,7 @@ export async function parseTraceRaw(file: string): Promise<{ events: any[], reso }; } -export async function parseTrace(file: string): Promise<{ resources: Map, events: (EventTraceEvent | ConsoleMessageTraceEvent)[], actions: ActionTraceEvent[], apiNames: string[], traceModel: TraceModel, model: MultiTraceModel, actionTree: string[] }> { +export async function parseTrace(file: string): Promise<{ resources: Map, events: (EventTraceEvent | ConsoleMessageTraceEvent)[], actions: ActionTraceEvent[], apiNames: string[], traceModel: TraceModel, model: MultiTraceModel, actionTree: string[], errors: string[] }> { const backend = new TraceBackend(file); const traceModel = new TraceModel(); await traceModel.load(backend, () => {}); @@ -174,6 +174,7 @@ export async function parseTrace(file: string): Promise<{ resources: Map e.message), model, traceModel, actionTree, diff --git a/tests/playwright-test/playwright.trace.spec.ts b/tests/playwright-test/playwright.trace.spec.ts index ec75ae0ef6..c62e7c98ca 100644 --- a/tests/playwright-test/playwright.trace.spec.ts +++ b/tests/playwright-test/playwright.trace.spec.ts @@ -323,7 +323,6 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve ' fixture: page', ' browserContext.newPage', 'page.goto', - 'error', 'After Hooks', ' fixture: page', ' fixture: context', @@ -335,6 +334,7 @@ test('should not override trace file in afterAll', async ({ runInlineTest, serve ' fixture: request', ' apiRequestContext.dispose', ]); + expect(trace1.errors).toEqual([`'oh no!'`]); const error = await parseTrace(testInfo.outputPath('test-results', 'a-test-2', 'trace.zip')).catch(e => e); expect(error).toBeTruthy(); @@ -668,11 +668,11 @@ test('should show non-expect error in trace', async ({ runInlineTest }, testInfo ' fixture: page', ' browserContext.newPage', 'expect.toBe', - 'ReferenceError: undefinedVariable1 is not defined', 'After Hooks', ' fixture: page', ' fixture: context', ]); + expect(trace.errors).toEqual(['ReferenceError: undefinedVariable1 is not defined']); }); test('should not throw when attachment is missing', async ({ runInlineTest }, testInfo) => { diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index 8090972125..aedc1e6eb7 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -820,7 +820,7 @@ for (const useIntermediateMergeReport of [false, true] as const) { await showReport(); await page.locator('text=sample').first().click(); - await expect(page.locator('text=ouch')).toHaveCount(2); + await expect(page.locator('text=ouch')).toHaveCount(1); await page.locator('text=All').first().click(); await page.locator('text=sample').nth(1).click();