diff --git a/packages/isomorphic/trace/traceModel.ts b/packages/isomorphic/trace/traceModel.ts index ea8381f910c26..9bfd61adfe612 100644 --- a/packages/isomorphic/trace/traceModel.ts +++ b/packages/isomorphic/trace/traceModel.ts @@ -167,12 +167,13 @@ export class TraceModel { private _errorDescriptorsFromActions(): ErrorDescription[] { const errors: ErrorDescription[] = []; for (const action of this.actions || []) { - if (!action.error?.message) + const message = actionErrorMessage(action); + if (!message) continue; errors.push({ action, stack: action.stack, - message: action.error.message, + message, }); } return errors; @@ -387,6 +388,17 @@ export function nextActionByStartTime(action: ActionTraceEvent): ActionTraceEven return (action as any)[nextByStartTimeSymbol]; } +// In library mode, `expect` returns a result instead of throwing, so its +// "failed" outcome never lands in `action.error`. Recognize that case here so +// the action shows up as an error in the trace viewer. +export function actionErrorMessage(action: ActionTraceEvent): string | undefined { + if (action.error?.message) + return action.error.message; + if (action.method === 'expect' && action.result && action.params && action.result.matches === action.params.isNot) + return 'Expect failed'; + return undefined; +} + export function stats(action: ActionTraceEvent): { errors: number, warnings: number } { let errors = 0; let warnings = 0; diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index c1c556e0fef73..eced23731533c 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -1455,11 +1455,6 @@ export class Frame extends SdkObject { async expect(progress: Progress, selector: string | undefined, options: FrameExpectParams): Promise { progress.log(`${renderTitleForCall(progress.metadata)}${options.timeoutForLogs ? ` with timeout ${options.timeoutForLogs}ms` : ''}`); const lastIntermediateResult: { received?: ExpectReceived, isSet: boolean, errorMessage?: string } = { isSet: false }; - const fixupMetadataError = (result: ExpectResult) => { - // Library mode special case for the expect errors which are return values, not exceptions. - if (result.matches === options.isNot) - progress.metadata.error = { error: { name: 'Expect', message: 'Expect failed' } }; - }; try { // Step 1: perform locator handlers checkpoint with a specified timeout. if (selector) @@ -1493,7 +1488,6 @@ export class Frame extends SdkObject { } return { matches, received }; }); - fixupMetadataError(result); return result; } catch (e) { // Q: Why not throw upon isNonRetriableError(e) as in other places? @@ -1509,7 +1503,6 @@ export class Frame extends SdkObject { } if (e instanceof TimeoutError) result.timedOut = true; - fixupMetadataError(result); return result; } } diff --git a/packages/playwright-core/src/server/recorder.ts b/packages/playwright-core/src/server/recorder.ts index c0a8c6dd9c606..653f09eaa979c 100644 --- a/packages/playwright-core/src/server/recorder.ts +++ b/packages/playwright-core/src/server/recorder.ts @@ -25,7 +25,7 @@ import { eventsHelper } from '@utils/eventsHelper'; import { monotonicTime } from '@isomorphic/time'; import { BrowserContext } from './browserContext'; import { Debugger } from './debugger'; -import { buildFullSelector, generateFrameSelector, metadataToCallLog } from './recorder/recorderUtils'; +import { buildFullSelector, generateFrameSelector, metadataError, metadataToCallLog } from './recorder/recorderUtils'; import { nullProgress, ProgressController } from './progress'; import { RecorderSignalProcessor } from './recorder/recorderSignalProcessor'; @@ -417,7 +417,7 @@ export class Recorder extends EventEmitter implements Instrume async onAfterCall(sdkObject: SdkObject, metadata: CallMetadata) { if (this._omitCallTracking || this._isRecording()) return; - if (!metadata.error) + if (!metadataError(metadata)) this._currentCallsMetadata.delete(metadata); this._updateUserSources(); this._updateCallLog([metadata]); @@ -442,7 +442,7 @@ export class Recorder extends EventEmitter implements Instrume } if (line) { const paused = this._debugger.isPaused(metadata); - source.highlight.push({ line, type: metadata.error ? 'error' : (paused ? 'paused' : 'running') }); + source.highlight.push({ line, type: metadataError(metadata) ? 'error' : (paused ? 'paused' : 'running') }); source.revealLine = line; } } diff --git a/packages/playwright-core/src/server/recorder/recorderUtils.ts b/packages/playwright-core/src/server/recorder/recorderUtils.ts index fca4472208ce3..31f9c456a5ecc 100644 --- a/packages/playwright-core/src/server/recorder/recorderUtils.ts +++ b/packages/playwright-core/src/server/recorder/recorderUtils.ts @@ -30,9 +30,21 @@ export function buildFullSelector(framePath: string[], selector: string) { return [...framePath, selector].join(' >> internal:control=enter-frame >> '); } +// In library mode, `expect` returns a result instead of throwing, so its +// "failed" outcome never lands in `metadata.error`. Recognize that case here so +// the action shows up as an error in the recorder UI. +export function metadataError(metadata: CallMetadata): string | undefined { + if (metadata.error?.error?.message) + return metadata.error.error.message; + if (metadata.method === 'expect' && metadata.result && metadata.params && metadata.result.matches === metadata.params.isNot) + return 'Expect failed'; + return undefined; +} + export function metadataToCallLog(metadata: CallMetadata, status: CallLogStatus): CallLog { const title = renderTitleForCall(metadata); - if (metadata.error) + const error = metadataError(metadata); + if (error) status = 'error'; const params = { url: metadata.params?.url, @@ -48,7 +60,7 @@ export function metadataToCallLog(metadata: CallMetadata, status: CallLogStatus) messages: metadata.log, title: title ?? '', status, - error: metadata.error?.error?.message, + error, params, duration, }; diff --git a/packages/trace-viewer/src/ui/actionList.tsx b/packages/trace-viewer/src/ui/actionList.tsx index 8f4e6ccaa321b..f23682d930ec2 100644 --- a/packages/trace-viewer/src/ui/actionList.tsx +++ b/packages/trace-viewer/src/ui/actionList.tsx @@ -19,7 +19,7 @@ import { clsx } from '@web/uiUtils'; import { msToString } from '@isomorphic/formatUtils'; import * as React from 'react'; import './actionList.css'; -import { stats, buildActionTree } from '@isomorphic/trace/traceModel'; +import { actionErrorMessage, stats, buildActionTree } from '@isomorphic/trace/traceModel'; import { asLocatorDescription, type Language } from '@isomorphic/locatorGenerators'; import type { TreeState } from '@web/components/treeView'; import { TreeView } from '@web/components/treeView'; @@ -71,7 +71,7 @@ export const ActionList: React.FC = ({ }, [itemMap, selectedAction]); const isError = React.useCallback((item: ActionTreeItem) => { - return !!item.action.error?.message; + return !!actionErrorMessage(item.action); }, []); const onAccepted = React.useCallback((item: ActionTreeItem) => { diff --git a/tests/library/inspector/pause.spec.ts b/tests/library/inspector/pause.spec.ts index a7c820343dbb6..74c5dfd61e43a 100644 --- a/tests/library/inspector/pause.spec.ts +++ b/tests/library/inspector/pause.spec.ts @@ -395,6 +395,27 @@ it.describe('pause', () => { expect(error.message).toContain('Not a checkbox or radio button'); }); + it('should populate log with expect failure', async ({ page, recorderPageGetter }) => { + await page.setContent(''); + const scriptPromise = (async () => { + // @ts-ignore + await page.pause({ __testHookKeepTestTimeout: true }); + await expect(page.getByRole('button')).toHaveText('Other', { timeout: 1 }); + })().catch(e => e); + const recorderPage = await recorderPageGetter(); + await recorderPage.click('[title="Resume (F8)"]'); + await recorderPage.waitForSelector('.source-line-error-underline'); + expect(await sanitizeLog(recorderPage)).toEqual([ + 'Pause- XXms', + 'Expect "toHaveText"(page.getByRole(\'button\'))- XXms', + 'Expect "toHaveText" with timeout 1ms', + 'waiting for getByRole(\'button\')', + 'error: Expect failed', + ]); + const error = await scriptPromise; + expect(error.message).toContain('toHaveText'); + }); + it('should populate log with error in waitForEvent', async ({ page, recorderPageGetter }) => { await page.setContent(''); const scriptPromise = (async () => {