[webkit-reviews] review denied: [Bug 169175] Web Inspector: Add tests for webSocketFrameSent and webSocketFrameReceived : [Attachment 306409] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 169175: Web Inspector: Add tests for webSocketFrameSent and
webSocketFrameReceived
https://bugs.webkit.org/show_bug.cgi?id=169175

Attachment 306409: Patch

https://bugs.webkit.org/attachment.cgi?id=306409&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 306409
  --> https://bugs.webkit.org/attachment.cgi?id=306409
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306409&action=review

The tests looks good but I want to see updated versions addressing most of my
comments.

Is there a test for `webSocketFrameError` in the works?

> LayoutTests/ChangeLog:9
> +	   Test recieving and sending text and binary (blob and array buffer)
data.

Typo: recieving

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary-expected.txt:4
> +== Running test suite: WebSocket.Binary

Do we have the size of the binary message? I think that would be useful.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary-expected.txt:17
> +PASS: Message walltime is greater than the first one.

A stricter test could check that the time is greater than the last message, not
the first message.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:11
> +    setTimeout(() => webSocket.close(), 500);

A timeout of 500ms could cause this to be flakey.

I'd suggest something like:

    // Global variable to keep it alive.
    let webSocket = null;

    function createWebSocket(type) {
	closeWebSocketConnection();
	webSocket = new WebSocket(...);
	webSocket.binaryType = type;
    }

    function closeWebSocketConnection() {
	if (webSocket) {
	    webSocket.close();
	    webSocket = null;
	}
    }

And at the end of `resourceWasAdded` before calling resolve you can
evaluateInPage(`closeWebSocketConnection()`) to clean up if you want.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:53
> +	   setTimeout(() => reject(), 500);

This will likely cause the test to be flakey. I'd suggest just dropping it.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:58
> +	   description: "WebInspector.WebSocketResource.Event.ReadyStateChanged
events are fired in order when WebSocket connection is closed by the client.",

This description doesn't match the test.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:61
> +

Nit: Drop this empty line.

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:62
> +	      
InspectorTest.evaluateInPage("createWebSocketConnection(\"blob\")");

Style: What I've tended to do is to use template strings for code, which
eliminates tedious escaping of quotes and it makes code strings appear
differently in good editors. So you would have:

    `createWebSocketConnection("blob")`

> LayoutTests/http/tests/websocket/tests/hybi/inspector/binary.html:68
> +	   description: "WebInspector.WebSocketResource.Event.ReadyStateChanged
events are fired in order when WebSocket connection is closed by the client.",

This description doesn't match the test.

>
LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive-expected
.txt:7
> +PASS: Message is a text type.

If its text we can show it!

>
LayoutTests/http/tests/websocket/tests/hybi/inspector/send-and-receive.html:27
> +	   description: "WebInspector.WebSocketResource.Event.ReadyStateChanged
events are fired in order when WebSocket connection is closed by the client.",

This description does not match the test.