-
Notifications
You must be signed in to change notification settings - Fork 4.8k
chore(bidi): add support for fetching request bodies #38212
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
962814b to
464a25d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| async body(): Promise<Buffer | null> { | ||
| if (this._overrides?.postData) | ||
| return this._overrides?.postData; | ||
| if (typeof this._postData === 'function') |
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.
Let's keep postDataBuffer as is and introduce a separate bodyCallback which will be non-null only in Bidi for now. This method should become something like this:
if (this._overrides?.postData)
return this._overrides?.postData;
if (this._bodyCallback)
return await this._bodyCallback();
return this._postData;
tests/page/interception.spec.ts
Outdated
| let intercepted = false; | ||
|
|
||
| await page.route(regexp, (route, request) => { | ||
| await page.route(regexp, async (route, request) => { |
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.
I don't think we want to change route handlers to async as it may change the order of the network events. At line 270 of this test we don't await continue to keep the callback sync.
| import { test as it, expect } from './pageTest'; | ||
|
|
||
| it('should return correct postData buffer for utf-8 body', async ({ page, server }) => { | ||
| it('should return correct postData buffer for utf-8 body', async ({ page, server, channel }) => { |
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.
You can skip entire file by adding top level it.skip(({ channel }) => channel?.startsWith('bidi-chrom') || channel?.startsWith('moz-firefox'), 'request.postData is not supported with BiDi'); instead of adding it to every test.
464a25d to
a0c3f27
Compare
Test results for "MCP"2 flaky2430 passed, 116 skipped Merge workflow run. |
Test results for "tests 1"1 failed 2 flaky40312 passed, 787 skipped Merge workflow run. |
|
Unfortunately the new API was reverted in #38281 as we couldn't find a single use that it was fixing in Playwright today. I tried switching to |
I have also updated the tests to use
request.body*instead ofrequest.postData*except for the following:tests/page/network-post-data.spec.tsto be skipped for BiDi because these tests are mirrored by those intests/page/network-request-body.spec.tstests/page/page-request-fallback.spec.tsbecause it only tests overridden request bodies which are always available synchronouslyFixes the following tests in Firefox:
tests/page/network-request-body.spec.ts:tests/page/page-network-request.spec.ts:tests/page/page-request-intercept.spec.ts:tests/page/page-route.spec.ts: