fix(runner): Properly attribute error messages to locator handlers #34074
Conversation
|
Please let me know if you have a better idea of how to plumb the overridden API name through the error reporting system. I'm not a fan of adding a new |
| await page.locator('ul > li').first().boundingBox(); | ||
|
|
||
| // Unreachable | ||
| expect(false).toBeTruthy(); |
There was a problem hiding this comment.
If there's a nicer way to handle testing for an exception, please let me know. I couldn't find a way for expect.toThrow() to be async.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"5 flaky37344 passed, 650 skipped Merge workflow run. |
| @@ -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; | |||
|
|
|||
There was a problem hiding this comment.
We should have a very good reason to change the fundamentals (channelOwner) for a niche DevX like the one in the bug. This isn't even an end-user reported feature, so I'd rather minimize the maintenance cost.
| public apiNameOverride: string; | ||
|
|
||
| constructor(error: Exclude<SerializedError['error'], undefined>, apiNameOverride: string) { | ||
| super(error.message); |
There was a problem hiding this comment.
We already have 4 layers of processing of the api names and errors, adding another one for this use case will add to the confusion.
There was a problem hiding this comment.
Oof, wasn't expecting this fix to become this big of a change! I see how that happens though. Here's a sketch for an alternative solution that doesn't require any protocol changes:
diff --git a/packages/playwright-core/src/client/channelOwner.ts b/packages/playwright-core/src/client/channelOwner.ts
index a5d753507..6615c0031 100644
--- a/packages/playwright-core/src/client/channelOwner.ts
+++ b/packages/playwright-core/src/client/channelOwner.ts
@@ -203,7 +203,10 @@ 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>'))
+ // some APIs have checkpoints that are executed within other APIs
+ if (e.message.startsWith('locatorHandler'))
+ e.message = 'locatorHandler: ' + e.message.substring(...);
+ else if (apiName && !apiName.includes('<anonymous>'))
e.message = apiName + ': ' + e.message;
const stackFrames = '\n' + stringifyStackFrames(stackTrace.frames).join('\n') + innerError;
if (stackFrames.trim())
diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts
index fe483b834..9c7b75179 100644
--- a/packages/playwright-core/src/server/page.ts
+++ b/packages/playwright-core/src/server/page.ts
@@ -466,7 +466,12 @@ export class Page extends SdkObject {
async performActionPreChecks(progress: Progress) {
await this._performWaitForNavigationCheck(progress);
progress.throwIfAborted();
+ try {
await this._performLocatorHandlersCheckpoint(progress);
+ } catch (e) {
+ e.message = 'locatorHandler: ' + e.message;
+ throw e;
+ }
progress.throwIfAborted();
// Wait once again, just in case a locator handler caused a navigation.
await this._performWaitForNavigationCheck(progress);This would do the job and be much smaller of a change. It also doesn't feel like much added complexity. What do y'all think?
Fix #34070. Provide a mechanism for overriding
apiNamein situations where the stack trace doesn't indicate the actual source of the test error.