-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for browserContext.on('pageerror') #24452
Conversation
@dgozman Thoughts? I am still trying to fix the docs and tests. |
Test results for "tests 1"12 failed 7 flaky 24943 passed, 583 skipped ❌ [chromium-ubuntu-22.04-node16] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [webkit-ubuntu-22.04-node16] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [webkit-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-20.04-chromium-tip-of-tree] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-20.04-chromium-tip-of-tree] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node20] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node20] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node18] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node18] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [firefox-ubuntu-22.04-node16] › library/browsercontext-pageerror-event.spec.ts:21:3 › should receive pageError in context
Retry 1:
Retry 2:
Retry 3:
❌ [firefox-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
Merge workflow run. |
We considered this, but so far there is no way to get the |
Ahh didn't know that was not possible. Would it be possible to arbitrary fetch the currentPage the context is on and also report that along with it?. Created a feature request #24466 |
1c874d5
to
3879b6e
Compare
Test results for "tests 1"7 failed 6 flaky 25009 passed, 583 skipped ❌ [chromium-ubuntu-22.04-node20] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-20.04-chromium-tip-of-tree] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node18] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [chromium-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [webkit-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [firefox-ubuntu-22.04-node16] › page/workers.spec.ts:90:3 › should report errors
Retry 1:
Retry 2:
Retry 3:
❌ [playwright-test-windows-latest-node16] › reporter-html.spec.ts:1452:7 › merged › labels › filter should update stats
Merge workflow run. |
935d1db
to
3d86575
Compare
Test results for "tests 1"4 interrupted 241 passed, 980 skipped Merge workflow run. |
Test results for "tests 1"4 interrupted 197 passed, 1024 skipped Merge workflow run. |
Test results for "tests 1"1 failed 5 flaky 25033 passed, 583 skipped ❌ [webkit-ubuntu-22.04-node16] › library/browsercontext-events.spec.ts:164:5 › pageError event should work
Retry 1:
Retry 2:
Retry 3:
Merge workflow run. |
Test results for "tests 1"1 failed 10 flaky 25028 passed, 583 skipped ❌ [playwright-test-macos-latest-node16] › expect-poll.spec.ts:222:5 › should show intermediate result for poll that spills over test time
Merge workflow run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good, just a few comments.
I think we should leave page as non-nullable in the protocol.yml
and as nullable in the api, like you already have.
Test results for "tests 1"1 failed 6 flaky 25037 passed, 583 skipped ❌ [playwright-test-windows-latest-node16] › reporter-html.spec.ts:1452:7 › merged › labels › filter should update stats
Merge workflow run. |
@vigneshshanmugam This looks great! This change will go into v1.38, so I will merge it once we cut v1.37. |
@dgozman :yay Thank you |
Co-authored-by: Max Schmitt <[email protected]> Signed-off-by: Vignesh Shanmugam <[email protected]>
Test results for "tests 1"1 failed 9 flaky 25060 passed, 583 skipped ❌ [playwright-test] › reporter-html.spec.ts:1491:7 › created › labels › filter should update stats
Merge workflow run. |
const pageObject = Page.from(page); | ||
const parsedError = parseError(error); | ||
this.emit(Events.BrowserContext.PageError, new PageError(pageObject, parsedError)); | ||
if (pageObject) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never null as otherwise Page.from would throw, remove this check.
+ fix microsoft#24466 + Adds support for exposing the `pageerror` events via `browserContext` API. + Helps with capturing the overall exceptions that are thrown outside of the the current page and also captures the exceptions happens on other windows/popups. + Keeps the API in sync with `context.on('request)', context.on('console'), etc..`
pageerror
events viabrowserContext
API.context.on('request)', context.on('console'), etc..