New issue
Advanced search Search tips

Issue 832535 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Is WebSocketHttp2HandshakeStream tested well?

Project Member Reported by yhirano@chromium.org, Apr 13 2018

Issue description

WebSocketBasicHandshakeStream is tested in websocket_stream_test.cc as WebSocket.*StreamCreate.*Test. Some of them such as WebSocketMultiProtocolStreamCreateTest cover both BASIC_HANDSHAKE_STREAM and HTTP2_HANDSHAKE_STREAM. Others such as WebSocketStreamCreateTest don't. Is WebSocketHttp2HandshakeStream tested well? Is it tested in other places?
 

Comment 1 by b...@chromium.org, Apr 13 2018

Components: Internals>Network>HTTP2
Excellent question.  There are indeed some unittests for the handshake in isolation, also some SpdyNetworkTransactionTests that test the entire WebSocket stream creation process.  Is there any particular functionality that you think should be tested more thoroughly?
I'm adding a WebRequest API support for WebSocket with Network Service (WIP: https://chromium-review.googlesource.com/c/chromium/src/+/1009266), and I found I cannot test part of the features because WebSocketStreamCreateTest::OnHeadersReceived I'm introducing in the CL uses CreateAndConnectCustomResponse.
Description: Show this description

Comment 4 by b...@chromium.org, Apr 13 2018

The semantics of the response headers is very different for HTTP/1 and HTTP/2 based WebSockets.  For example, 101 and 200 status codes mean success, respectively.  There is also a difference is some other response headers. 
 Not to mention that the syntax of response headers is different for the different HTTP versions.  CreateAndConnectCustomResponse is only used for HTTP/1-specific tests for this reason.

For this exact reason, are you sure it is a good idea to expose response headers to developers?  They would then need to deal with different semantics depending on HTTP protocol version, and I foresee a lot of applications hardwired not to support HTTP/2.

Otherwise, for the OnHeadersReceived test, you can call SetHttp2ResponseStatus(404) before calling CreateAndConnectStandard() to generate mock read data with 404 error.
Fair enough. I don't know how they should be exposed either.

Sign in to add a comment