fix(routeFromHAR, take 2): hack harRouter to merge set-cookie headers#38487
fix(routeFromHAR, take 2): hack harRouter to merge set-cookie headers#38487yury-s merged 4 commits intomicrosoft:mainfrom
Conversation
Route.fulfill currently does not support multiple headers with the same name (microsoft#37342). There are workarounds when using this API directly (merging headers, tweaking the header name casing, etc.), but this is problematic for routeFromHAR, which depends on this API internally. This patch adds special handling for set-cookie headers within harRouter to merge them into one header. There's some precedent for treating set-cookie specially at various places in the codebase (ex: https://github.com/microsoft/playwright/blob/f54478a23e0daa450fe524905eabc8aabf6efb07/packages/playwright-core/src/utils/isomorphic/headers.ts#L29, https://github.com/microsoft/playwright/blob/baeb065e9ea84502f347129a0b896a85d2a8dada/packages/playwright-core/src/server/chromium/crNetworkManager.ts#L675), so I think this is okay.
| expect(await page2.evaluate(fetchFunction, { path: '/echo', body: '12' })).toBe('12'); | ||
| }); | ||
|
|
||
| it('should replay requests with multiple set-cookie headers properly', async ({ context, asset }) => { |
There was a problem hiding this comment.
Not sure this test is still necessary with the multi-cookie test below (seems like that one both records and replays a HAR)
There was a problem hiding this comment.
Let's remove it and revert the manual changes to har-fulfill.har
| expect(cookie2.split('; ').sort().join('; ')).toBe('first=foo'); | ||
| }); | ||
|
|
||
| it('should record multiple set-cookie headers', { |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| expect(await page2.evaluate(fetchFunction, { path: '/echo', body: '12' })).toBe('12'); | ||
| }); | ||
|
|
||
| it('should replay requests with multiple set-cookie headers properly', async ({ context, asset }) => { |
There was a problem hiding this comment.
Let's remove it and revert the manual changes to har-fulfill.har
Co-authored-by: Yury Semikhatsky <yurys@chromium.org> Signed-off-by: Victor Lin <125177+yepitschunked@users.noreply.github.com>
|
@yury-s took me a while due to the holidays - addressed your feedback on the tests |
There was a problem hiding this comment.
please revert changes in this file and we'll merge this PR
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"8 flaky34234 passed, 756 skipped Merge workflow run. |
Test results for "MCP"10 failed 4678 passed, 135 skipped Merge workflow run. |
This was previously closed in #38255 due to inactivity. This PR has been updated with the feedback from @yury-s on the previous PR to avoid a bug with single set-cookie headers.
Following PR description is copied from the original:
Route.fulfill currently does not support multiple headers with the same name (#37342). There are workarounds when using this API directly (merging headers, tweaking the header name casing, etc.), but this is problematic for routeFromHAR, which depends on
this API internally. This patch adds special handling for set-cookie headers within harRouter to merge them into one header. There's some precedent for treating set-cookie specially at various places in the codebase (ex:
playwright/packages/playwright-core/src/utils/isomorphic/headers.ts
Line 29 in f54478a
playwright/packages/playwright-core/src/server/chromium/crNetworkManager.ts
Line 675 in baeb065