Bug 169175 – Web Inspector: Add tests for webSocketFrameSent and webSocketFrameReceived

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

NetworkObserver#webSocketFrameReceived and NetworkObserver#webSocketFrameSent were implemented in Bug 167520: Web Inspector: Show Web Socket connections in Network tab.

These two methods are currently untested.

[Radar WebKit Bug Importer]

<rdar://problem/30853154>

[Nikita Vasilyev]

Created attachment 306409 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 306409 [details]
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.

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #3)
> Comment on attachment 306409 [details]
> 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?

Yes. It's more complicated since there're many reasons a connection may fail. I'm planning on adding webSocketFrameError tests at the same time as the UI for displaying these errors in WebSocketContentView, separately from this patch.

[Joseph Pecoraro]

> Yes. It's more complicated since there're many reasons a connection may
> fail. I'm planning on adding webSocketFrameError tests at the same time as
> the UI for displaying these errors in WebSocketContentView, separately from
> this patch.

Okay! Can you point me to the bug covering that? Thanks

[Nikita Vasilyev]

(In reply to Joseph Pecoraro from comment #5)
> > Yes. It's more complicated since there're many reasons a connection may
> > fail. I'm planning on adding webSocketFrameError tests at the same time as
> > the UI for displaying these errors in WebSocketContentView, separately from
> > this patch.
> 
> Okay! Can you point me to the bug covering that? Thanks

<rdar://problem/31389605>

[Nikita Vasilyev]

Comment on attachment 306409 [details]
Patch

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

>> 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.

Yes, but it was incorrect.
Bug 170609 - Web Inspector: WebSockets: Transferred size is incorrect.

[Blaze Burg]

<rdar://problem/30853154>

[Nikita Vasilyev]

Comment on attachment 306409 [details]
Patch

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

>> 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!

I'm not sure what you're suggesting.

    InspectorTest.expectEqual(frame.opcode, WebInspector.WebSocketResource.OpCodes.TextFrame, frame.data);

?

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #9)
> Comment on attachment 306409 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306409&action=review
> 
> >> 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!
> 
> I'm not sure what you're suggesting.
> 
>     InspectorTest.expectEqual(frame.opcode,
> WebInspector.WebSocketResource.OpCodes.TextFrame, frame.data);
> 
> ?

    InspectorTest.expectEqual(frame.opcode, OpCodes.TextFrame, "Frame should be text");
    InspectorTest.expectEqual(frame.data, "Hello world", "Frame data should be 'Hello world'");

[Nikita Vasilyev]

Fixed in https://trac.webkit.org/changeset/215388/webkit.