From 1ac1ca64bf4260952512de13b0d80ea8b3c5f595 Mon Sep 17 00:00:00 2001 From: Adam Gastineau Date: Wed, 18 Dec 2024 11:15:57 -0800 Subject: [PATCH 1/2] fix(runner): Properly attribute error messages to locator handlers --- .../src/client/channelOwner.ts | 10 ++-- packages/playwright-core/src/client/errors.ts | 21 ++++++++ packages/playwright-core/src/server/errors.ts | 39 ++++++++++++++- packages/playwright-core/src/server/page.ts | 50 +++++++++++-------- packages/protocol/src/channels.ts | 1 + tests/page/page-add-locator-handler.spec.ts | 16 ++++++ 6 files changed, 110 insertions(+), 27 deletions(-) diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts index a5d753507bddf..f170d92dcdbbe 100644 --- a/packages/playwright-core/src/client/channelOwner.ts +++ b/packages/playwright-core/src/client/channelOwner.ts @@ -25,6 +25,7 @@ import { zones } from '../utils/zones'; import type { ClientInstrumentation } from './clientInstrumentation'; import type { Connection } from './connection'; import type { Logger } from './types'; +import { isOverriddenAPIError } from './errors'; type Listener = (...args: any[]) => void; @@ -203,15 +204,18 @@ export abstract class ChannelOwner\n' + e.stack : ''; - if (apiName && !apiName.includes('')) - e.message = apiName + ': ' + e.message; + + const computedAPIName = isOverriddenAPIError(e) ? e.apiNameOverride : apiName; + + if (computedAPIName && !computedAPIName.includes('')) + e.message = computedAPIName + ': ' + e.message; const stackFrames = '\n' + stringifyStackFrames(stackTrace.frames).join('\n') + innerError; if (stackFrames.trim()) e.stack = e.message + stackFrames; else e.stack = ''; csi?.onApiCallEnd(callCookie, e); - logApiCall(logger, `<= ${apiName} failed`, isInternal); + logApiCall(logger, `<= ${computedAPIName} failed`, isInternal); throw e; } } diff --git a/packages/playwright-core/src/client/errors.ts b/packages/playwright-core/src/client/errors.ts index 51555202e5b84..ff665930fb892 100644 --- a/packages/playwright-core/src/client/errors.ts +++ b/packages/playwright-core/src/client/errors.ts @@ -35,6 +35,21 @@ export function isTargetClosedError(error: Error) { return error instanceof TargetClosedError; } +export class OverriddenAPIError extends Error { + public apiNameOverride: string; + + constructor(error: Exclude, apiNameOverride: string) { + super(error.message); + this.name = error.name; + this.stack = error.stack; + this.apiNameOverride = apiNameOverride; + } +} + +export function isOverriddenAPIError(error: Error) { + return error instanceof OverriddenAPIError; +} + export function serializeError(e: any): SerializedError { if (isError(e)) return { error: { message: e.message, stack: e.stack, name: e.name } }; @@ -47,6 +62,12 @@ export function parseError(error: SerializedError): Error { throw new Error('Serialized error must have either an error or a value'); return parseSerializedValue(error.value, undefined); } + if (error.error.apiNameOverride) { + const e = new OverriddenAPIError(error.error, error.error.apiNameOverride); + e.stack = error.error.stack; + e.name = error.error.name; + return e; + } if (error.error.name === 'TimeoutError') { const e = new TimeoutError(error.error.message); e.stack = error.error.stack || ''; diff --git a/packages/playwright-core/src/server/errors.ts b/packages/playwright-core/src/server/errors.ts index c3a63cb0339fe..209521bc2e625 100644 --- a/packages/playwright-core/src/server/errors.ts +++ b/packages/playwright-core/src/server/errors.ts @@ -17,6 +17,7 @@ import type { SerializedError } from '@protocol/channels'; import { isError } from '../utils'; import { parseSerializedValue, serializeValue } from '../protocol/serializers'; +import { JavaScriptErrorInEvaluate } from './javascript'; class CustomError extends Error { constructor(message: string) { @@ -37,9 +38,43 @@ export function isTargetClosedError(error: Error) { return error instanceof TargetClosedError || error.name === 'TargetClosedError'; } +export class OverriddenAPIError extends JavaScriptErrorInEvaluate { + public apiNameOverride: string; + + constructor(error: JavaScriptErrorInEvaluate, apiNameOverride: string) { + super(error.message); + this.name = error.name; + this.stack = error.stack; + this.apiNameOverride = apiNameOverride; + } +} + +export function isOverriddenAPIError(error: Error) { + return error instanceof OverriddenAPIError; +} + export function serializeError(e: any): SerializedError { - if (isError(e)) - return { error: { message: e.message, stack: e.stack, name: e.name } }; + if (isError(e)) { + const serializedError: { + error: { + message: any, + stack: any, + name: string, + apiNameOverride?: string; + } + } = { + error: { + message: e.message, + stack: e.stack, + name: e.name + } + }; + + if (isOverriddenAPIError(e)) + serializedError.error.apiNameOverride = e.apiNameOverride; + + return serializedError; + } return { value: serializeValue(e, value => ({ fallThrough: value })) }; } diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index fe483b8347945..84892d8860dd0 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -43,7 +43,7 @@ import type { TimeoutOptions } from '../common/types'; import { isInvalidSelectorError } from '../utils/isomorphic/selectorParser'; import { parseEvaluationResultValue, source } from './isomorphic/utilityScriptSerializers'; import type { SerializedValue } from './isomorphic/utilityScriptSerializers'; -import { TargetClosedError, TimeoutError } from './errors'; +import { OverriddenAPIError, TargetClosedError, TimeoutError } from './errors'; import { asLocator } from '../utils'; import { helper } from './helper'; @@ -494,30 +494,36 @@ export class Page extends SdkObject { // Do not run locator handlers from inside locator handler callbacks to avoid deadlocks. if (this._locatorHandlerRunningCounter) return; - for (const [uid, handler] of this._locatorHandlers) { - if (!handler.resolved) { - if (await this.mainFrame().isVisibleInternal(handler.selector, { strict: true })) { - handler.resolved = new ManualPromise(); - this.emit(Page.Events.LocatorHandlerTriggered, uid); + try { + for (const [uid, handler] of this._locatorHandlers) { + if (!handler.resolved) { + if (await this.mainFrame().isVisibleInternal(handler.selector, { strict: true })) { + handler.resolved = new ManualPromise(); + this.emit(Page.Events.LocatorHandlerTriggered, uid); + } } - } - if (handler.resolved) { - ++this._locatorHandlerRunningCounter; - progress.log(` found ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)}, intercepting action to run the handler`); - const promise = handler.resolved.then(async () => { + if (handler.resolved) { + ++this._locatorHandlerRunningCounter; + progress.log(` found ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)}, intercepting action to run the handler`); + const promise = handler.resolved.then(async () => { + progress.throwIfAborted(); + if (!handler.noWaitAfter) { + progress.log(` locator handler has finished, waiting for ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)} to be hidden`); + await this.mainFrame().waitForSelectorInternal(progress, handler.selector, false, { state: 'hidden' }); + } else { + progress.log(` locator handler has finished`); + } + }); + await this.openScope.race(promise).finally(() => --this._locatorHandlerRunningCounter); + // Avoid side-effects after long-running operation. progress.throwIfAborted(); - if (!handler.noWaitAfter) { - progress.log(` locator handler has finished, waiting for ${asLocator(this.attribution.playwright.options.sdkLanguage, handler.selector)} to be hidden`); - await this.mainFrame().waitForSelectorInternal(progress, handler.selector, false, { state: 'hidden' }); - } else { - progress.log(` locator handler has finished`); - } - }); - await this.openScope.race(promise).finally(() => --this._locatorHandlerRunningCounter); - // Avoid side-effects after long-running operation. - progress.throwIfAborted(); - progress.log(` interception handler has finished, continuing`); + progress.log(` interception handler has finished, continuing`); + } } + } catch (e) { + // Since this checkpoint occurs during the evaluation of a different public API call, any errors are automatically attributed + // to that API call, rather than the locator handler. Mark the error from the locator handler + throw new OverriddenAPIError(e, 'page.addLocatorHandler'); } } diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 100baf59f9601..e58694bfb7918 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -281,6 +281,7 @@ export type SerializedError = { message: string, name: string, stack?: string, + apiNameOverride?: string, }, value?: SerializedValue, }; diff --git a/tests/page/page-add-locator-handler.spec.ts b/tests/page/page-add-locator-handler.spec.ts index f8b1bb6ba720e..c5cc06928de49 100644 --- a/tests/page/page-add-locator-handler.spec.ts +++ b/tests/page/page-add-locator-handler.spec.ts @@ -371,3 +371,19 @@ test('should removeLocatorHandler', async ({ page, server }) => { await expect(page.locator('#interstitial')).toBeVisible(); expect(error.message).toContain('Timeout 3000ms exceeded'); }); + +test('should attribute errors to the locator handler', async ({ page }) => { + await page.setContent(``); + + await page.addLocatorHandler(page.locator('ul'), async locator => Promise.resolve()); + + try { + // Perform an operation that will trigger the locator handler + await page.locator('ul > li').first().boundingBox(); + + // Unreachable + expect(false).toBeTruthy(); + } catch (e) { + expect(e.message).toContain(`page.addLocatorHandler: Error: strict mode violation: locator('ul') resolved to 2 elements:`); + } +}); \ No newline at end of file From f33cda0925db2f1708ac07a854a450f7d34830d6 Mon Sep 17 00:00:00 2001 From: Adam Gastineau Date: Wed, 18 Dec 2024 11:31:03 -0800 Subject: [PATCH 2/2] Fixed incorrectly changed protocol --- packages/playwright-core/src/protocol/validator.ts | 1 + packages/protocol/src/protocol.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/playwright-core/src/protocol/validator.ts b/packages/playwright-core/src/protocol/validator.ts index f4db833e02387..ca82962db1018 100644 --- a/packages/playwright-core/src/protocol/validator.ts +++ b/packages/playwright-core/src/protocol/validator.ts @@ -151,6 +151,7 @@ scheme.SerializedError = tObject({ message: tString, name: tString, stack: tOptional(tString), + apiNameOverride: tOptional(tString), })), value: tOptional(tType('SerializedValue')), }); diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index 9997989f77cef..6566a7238db8e 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -241,6 +241,7 @@ SerializedError: message: string name: string stack: string? + apiNameOverride: string? value: SerializedValue?