feat(api): add Response.httpVersion() method#39434
Conversation
yury-s
left a comment
There was a problem hiding this comment.
The PR still has too many issues and lacking any tests. Unfortunately, I don't see it converging.
| if (httpVersion === 'h2') | ||
| return 'HTTP/2.0'; | ||
| return this._httpVersion; | ||
| if (httpVersion === 'h3') |
There was a problem hiding this comment.
this does not improve things and just adds confusion, don't change it
| this._getResponseBodyCallback = getResponseBodyCallback; | ||
| this._request._setResponse(this); | ||
| this._httpVersion = httpVersion; | ||
| if (httpVersion) |
There was a problem hiding this comment.
only one client has it ready right away, make it call the new setter and remove the param
| } | ||
|
|
||
| private _onResponse(response: network.Response) { | ||
| private async _onResponse(response: network.Response) { |
There was a problem hiding this comment.
we don't want async event handlers
| async httpVersion(): Promise<string> { | ||
| return this._wrapApiCall(async () => { | ||
| return (await this._channel.httpVersion()).value; | ||
| }, { internal: true }); |
There was a problem hiding this comment.
why do you have this here while it's already defined in the protocol yaml?
There was a problem hiding this comment.
Your 100% right, my bad, I naively dived into this PR before understanding enough and misunderstood the architecture. Once I found the protocol.yml I added the definition there, but forgot to revert this change
I've hopefully resolved your comments and added a basic test. I'm willing get this to a place you guys are happy with but if that's never going to happen, feel free to close the PR. However, not having this feature would be a shame since the only way to assert the httpVersion using HAR recording is currently awkward/heavy |
| }); | ||
| if (event.metrics?.protocol) | ||
| response._setHttpVersion(event.metrics.protocol); | ||
| response._setHttpVersion(event.metrics?.protocol ?? null); |
There was a problem hiding this comment.
We should call this from _onLoadingFailed as well, similar to calling _securityDetailsFinished().
| } | ||
| if (event.protocolVersion) | ||
| response._setHttpVersion(event.protocolVersion); | ||
| response._setHttpVersion(event.protocolVersion ?? null); |
There was a problem hiding this comment.
Call this from _onRequestFailed as well.
| const httpVersion = response.httpVersion(); | ||
| harEntry.request.httpVersion = httpVersion; | ||
| harEntry.response.httpVersion = httpVersion; | ||
| this._addBarrier(page || request.serviceWorker(), response.httpVersion().then(httpVersion => { |
There was a problem hiding this comment.
We don't need two barriers for http version. Let's only have a single one in _onResponse() that sets both request and response http versions.
| import { expect, test as it } from './pageTest'; | ||
| import { TestServer } from '../config/testserver'; | ||
|
|
||
| const { createHttp2Server } = require('../../packages/playwright-core/lib/utils'); |
| expect(request.existingResponse()).toBe(response); | ||
| }); | ||
|
|
||
| it('should return http version', async ({ page, server }) => { |
There was a problem hiding this comment.
Let's add a test for a failing request - httpVersion() should not hang in this case. Consult Page.Events.RequestFailed from page-event-network.spec.ts to see how to trigger a failing request.
There was a problem hiding this comment.
Based on that test you mentioned, it shows that the Response object should be null if the request fails. This PR only exposes the httpVersion method on the response. Not sure how I'd test that exactly since the Response should be null.
I did briefly consider adding a httpVersion method to the Request but it didn't feel like it made sense. Since as far as I can tell, it's not available until a response payload is received anyway.
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"7 failed 5 flaky38690 passed, 841 skipped Merge workflow run. |
Test results for "MCP"2 failed 5209 passed, 171 skipped Merge workflow run. |
dgozman
left a comment
There was a problem hiding this comment.
Looks great, thank you for the PR!
| expect(request.existingResponse()).toBe(response); | ||
| }); | ||
|
|
||
| it('should return http version', async ({ page, server }) => { |
For hanging requests in Firefox/WebKit, the HTTP version promise never resolves, causing HAR flush to hang indefinitely. Remove from barrier promises while keeping the non-blocking update. Fixes issues introduced in microsoft#39434
Adds
Response.httpVersion()to get the http version of a given response.Issue: #39310
Previous PR #39309 was closed out with some comments. I've addressed the comments and opened a this new PR. This feature would be extremely useful to reduce bloat/boilerplate for our test suite.