fix(runner): Properly attribute error messages to locator handlers

microsoft · Dec 18, 2024 · 1ac1ca6 · 1ac1ca6
1 parent aabbcbf
commit 1ac1ca6
Show file tree

Hide file tree

Showing 6 changed files with 110 additions and 27 deletions.
diff --git 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<T extends channels.Channel = channels.Channel
       return result;
     } catch (e) {
       const innerError = ((process.env.PWDEBUGIMPL || isUnderTest()) && e.stack) ? '\n<inner error>\n' + e.stack : '';
-      if (apiName && !apiName.includes('<anonymous>'))
-        e.message = apiName + ': ' + e.message;
+
+      const computedAPIName = isOverriddenAPIError(e) ? e.apiNameOverride : apiName;
+
+      if (computedAPIName && !computedAPIName.includes('<anonymous>'))
+        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
@@ -35,6 +35,21 @@ export function isTargetClosedError(error: Error) {
   return error instanceof TargetClosedError;
 }
 
+export class OverriddenAPIError extends Error {
+  public apiNameOverride: string;
+
+  constructor(error: Exclude<SerializedError['error'], undefined>, 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
@@ -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
@@ -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
@@ -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
@@ -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(`<html><body><script>setTimeout(() => {document.body.innerHTML = '<div><ul><li>Foo</li></ul><ul><li>Bar</li></ul></div>'}, 100)</script></body></html>`);
+
+  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:`);
+  }
+});