Fix some WebSocket test handlers#7345
Conversation
Several of the pywebsocket handlers used in tests had the problem that they would generate a hand-crafted header and then pywebsocket would add its own header immediately afterwards, causing unnecessary errors. In most cases there was no need to generate a hand-crafted header at all, so the tests have been modified to use pywebsocket APIs to do the same thing. simple_handshake_wsh.py still generates a hand-crafted header but now suppresses pywebsocket's own header. Several tests closed the WebSocket on the client side but the handler didn't wait to receive the client-side close. These have been modified to wait for the close frame. Several tests had expectations that onclose would never be called but then called close() themselves, causing onclose to be called. In some cases this was mitigated by calling t.done() before t.unreached_func() was called, but it has been fixed by disabling the onclose handler after calling close(). This change only fixes a subset of handlers and their affected tests. It is not intended to be comprehensive, and many issues remain.
Build PASSEDStarted: 2017-10-04 06:05:49 View more information about this build on: |
|
@tyoshino PTAL |
I don't think these particular changes are necessary or helpful. |
I have two problems with the way this works at the moment:
|
|
This is very much part of the design of the test harness; perhaps @gsnedders has thoughts. |
|
I've never touched any of this; defer to @jgraham. |
tyoshino
left a comment
There was a problem hiding this comment.
Even if the design guarantees that, I'd like to vote for the change considering ricea's second point.
|
|
||
| from mod_pywebsocket import common, msgutil, util | ||
| from mod_pywebsocket.handshake import hybi | ||
| import struct |
There was a problem hiding this comment.
Good catch. Must have been leftover from a previous iteration.
| # to use the stream methods at this point because the stream hasn't been | ||
| # established from pywebsocket's point of view. Instead just read the | ||
| # appropriate number of bytes and assume they are correct. | ||
| request.connection.read(8) |
There was a problem hiding this comment.
Don't we need to emulate _stream_base.receive_bytes() here?
There was a problem hiding this comment.
I think it's safe to assume that if we got any bytes at all then the close frame has been sent.
It's possible that a browser could send the close frame fragmented into two parts, the server closes the connection after the first fragment and the browser gets a "Connection reset by peer" error and sets wasClean to false. But I know of no reason why a browser would fragment the close frame.
This handler is already complicated and probably pointless, so I don't want to complicate it any further.
tyoshino
left a comment
There was a problem hiding this comment.
I'm happy with the current patch. You can wait for jgraham's comment regarding usage of done().
|
@jgraham, do you have any comments on whether asserting after a async_test is done is good practice? |
|
@ricea I'd hope we ignore such asserts? Probably even set the test/harness to ERROR. |
|
@gsnedders The asserts were ignored. I found it surprising. I see the situation with uncaught promise rejections in promise_tests as analogous. Uncaught rejections cause the whole page to fail, even though they can't be attributed to a single test. |
|
@ricea Likewise uncaught exceptions do. We should definitely fix this. Lemme file a bug. |
|
Oh, thinking about this, it may actually be unsolvable as long as assertions aren't linked directly to tests and only are processed as a result of them throwing an exception. We definitely can't ever catch |
Several of the pywebsocket handlers used in tests had the problem that they
would generate a hand-crafted header and then pywebsocket would add its own
header immediately afterwards, causing unnecessary errors.
In most cases there was no need to generate a hand-crafted header at all, so the
tests have been modified to use pywebsocket APIs to do the same
thing. simple_handshake_wsh.py still generates a hand-crafted header but now
suppresses pywebsocket's own header.
Several tests closed the WebSocket on the client side but the handler didn't
wait to receive the client-side close. These have been modified to wait for the
close frame.
Several tests had expectations that onclose would never be called but then
called close() themselves, causing onclose to be called. In some cases this was
mitigated by calling t.done() before t.unreached_func() was called, but it has
been fixed by disabling the onclose handler after calling close().
This change only fixes a subset of handlers and their affected tests. It is not
intended to be comprehensive, and many issues remain.