Skip to content
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

Wpt flags h2 #27790

Merged
merged 2 commits into from Mar 19, 2021
Merged

Wpt flags h2 #27790

merged 2 commits into from Mar 19, 2021

Conversation

@arenevier
Copy link
Contributor

@arenevier arenevier commented Feb 25, 2021

No description provided.

@arenevier
Copy link
Contributor Author

@arenevier arenevier commented Feb 25, 2021

websockets/constants.js Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@
from mod_pywebsocket import msgutil

def web_socket_do_extra_handshake(request):
request.ws_cookie = request.headers_in.get('Cookie')
request.ws_cookie = request.headers_in.get('Cookie') or request.headers_in.get('cookie')

This comment has been minimized.

@ricea

ricea Feb 26, 2021
Contributor

It seems weird that you need to do this. I file GoogleChromeLabs/pywebsocket3#18 but I don't know whether we'll fix it there.

This comment has been minimized.

@arenevier

arenevier Feb 26, 2021
Author Contributor

in the case of ws over h2, headers_in are of type H2Headers

class H2Headers(dict):

The alternative would be to modify H2Headers to match fields case-insensitively, though I'm worried of breaking other tests.

This comment has been minimized.

@ricea

ricea Feb 26, 2021
Contributor

I see, thanks. Maybe experiment with changing H2Headers in a separate CL?

This comment has been minimized.

@ricea

ricea Feb 26, 2021
Contributor

Actually, I think we can get away with only the lower case versions, since if we are doing a case-sensitive comparison, we must be using H2.

This comment has been minimized.

@arenevier

arenevier Feb 27, 2021
Author Contributor

Good idea. I have uploaded a new patch.

@arenevier arenevier force-pushed the arenevier:wpt_flags_h2 branch from 3f53a7a to fae94c2 Feb 26, 2021
@ricea
ricea approved these changes Feb 26, 2021
Copy link
Contributor

@ricea ricea left a comment

lgtm, great, thanks!

arenevier added 2 commits Feb 25, 2021
Right now, wss variants will fail because they are not served over
https. And an insecure page cannot set a secure cookie.

As a solution, we load those variants with wpt_flags=https. It will
force the test to be loaded over https.

006.https.html was a previous way of solving the same issue. We can now
get rid of it, and simply use 006.html with the correct variant set.
All those tests have been checked to pass manually (or, if the wss
variant fails, to fail in the same way)

Some websocket handlers needed to be modified to accept lower case
headers.

We also need to fix detection of h2 in constants.js: h2 is not specified
with h2 parameter, but with wpt_flags=h2 parameter.
@arenevier arenevier force-pushed the arenevier:wpt_flags_h2 branch from fae94c2 to 92e6b01 Feb 27, 2021
@arenevier
Copy link
Contributor Author

@arenevier arenevier commented Mar 10, 2021

@jdm @zqzhang : Can you take a look?

@jgraham jgraham merged commit 29c9c81 into web-platform-tests:master Mar 19, 2021
21 checks passed
21 checks passed
@github-actions
update-pr-preview
Details
@azure-pipelines
Azure Pipelines Build #20210227.14 succeeded
Details
@azure-pipelines
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
@azure-pipelines
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
@community-tc-integration
download-firefox-nightly Community-TC (pull_request)
Details
@community-tc-integration
lint Community-TC (pull_request)
Details
@community-tc-integration
sink-task Community-TC (pull_request)
Details
@community-tc-integration
update-built Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-chrome-dev-stability Community-TC (pull_request)
Details
@community-tc-integration
wpt-decision-task Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-results-without-changes Community-TC (pull_request)
Details
@community-tc-integration
wpt-firefox-nightly-stability Community-TC (pull_request)
Details
@wpt-fyi
wpt.fyi - chrome[experimental] Chrome results
Details
@wpt-fyi
wpt.fyi - firefox[experimental] Firefox results
Details
@wpt-fyi
wpt.fyi - safari[experimental] Safari results
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants