Bug 215848 - Remove console logs from expected.txt files for some WebSocket tests
Summary: Remove console logs from expected.txt files for some WebSocket tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-26 03:51 PDT by youenn fablet
Modified: 2020-08-27 21:49 PDT (History)
3 users (show)

See Also:


Attachments
Patch (83.22 KB, patch)
2020-08-26 03:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (112.71 KB, patch)
2020-08-27 03:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2020-08-26 03:51:24 PDT
Some WebSocket tests show platform specific console log messages.
Comment 1 youenn fablet 2020-08-26 03:52:44 PDT
Created attachment 407288 [details]
Patch
Comment 2 Alex Christensen 2020-08-26 09:57:33 PDT
Comment on attachment 407288 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        By removing them, we can keep one expected.txt file for all platforms.

I don't see removal of expected.txt files in this patch.  What platforms and what tests?
This does reduce useful test behavior checking.  Why is that worth it?
Comment 3 youenn fablet 2020-08-26 10:19:07 PDT
> I don't see removal of expected.txt files in this patch.  What platforms and
> what tests?

There is no removal for now but this would allow keeping the same expected file for GTK, macOS Catalina, MacOS BigSur with NSURLSession code path and iOS.

I believe several GTK/GLib web sockets expected.txt files could be removed.
See for instance LayoutTests/http/tests/websocket/tests/hybi/long-control-frame-expected.txt and LayoutTests/platform/glib/http/tests/websocket/tests/hybi/long-control-frame-expected.txt.
I haven't done it since there is no GTK EWS bot.
When we enable NSURLSession code path, this will allow to not create specific expectations for BigSur/iOS.

> This does reduce useful test behavior checking.  Why is that worth it?

The benefit of removing this logging is that we keep it easy to manage the inconsistencies between all the platforms.
If we do a change to WebSocket legacy code path, it will be super easy to see whether it is consistent with GTK or NSURLSession code paths.

The benefit of keeping the console logging is mostly that we can validate that the test is testing what it is supposed to be testing. But it does not bring much coverage on the source code in itself.
Comment 4 Alex Christensen 2020-08-26 10:43:45 PDT
Is there a reason we can't just make the logs for the NSURLSession and CFStream code paths line up?
Comment 5 youenn fablet 2020-08-26 10:53:16 PDT
(In reply to Alex Christensen from comment #4)
> Is there a reason we can't just make the logs for the NSURLSession and
> CFStream code paths line up?

That would be difficult. CFStream code path logging is usually more precise, but not always.

Some examples:
-CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/interleaved-fragments' failed: Received new data frame but previous continuous frame is unfinished.
+CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/interleaved-fragments' failed: The operation couldn’t be completed. (kNWErrorDomainPOSIX error 57 - Socket is not connected)

-CONSOLE MESSAGE: WebSocket network error: The operation couldn’t be completed. Connection refused
+CONSOLE MESSAGE: WebSocket connection to 'ws://localhost/' failed: Could not connect to the server.

-CONSOLE MESSAGE: WebSocket connection to 'ws://127.0.0.1:8880/websocket/tests/hybi/broken-utf8' failed: Could not decode a text frame as UTF-8.
+CONSOLE MESSAGE: WebSocket connection to 'ws://127.0.0.1:8880/websocket/tests/hybi/broken-utf8' failed: The operation couldn’t be completed. (kNWErrorDomainPOSIX error 96 - No message available on STREAM)

-CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/deflate-frame-invalid-parameter?x-foo' failed: Received unexpected deflate-frame parameter
+CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/deflate-frame-invalid-parameter?x-foo' failed: There was a bad response from the server.

-CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/handshake-fail-by-extensions-header' failed: Received unexpected extension: x-foo
+CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/handshake-fail-by-extensions-header' failed: There was a bad response from the server.
Comment 6 Alex Christensen 2020-08-26 10:54:25 PDT
Comment on attachment 407288 [details]
Patch

Ah, ok.  Please try to remove any identical files, at least.
Comment 7 youenn fablet 2020-08-27 03:05:11 PDT
Created attachment 407387 [details]
Patch for landing
Comment 8 youenn fablet 2020-08-27 03:07:06 PDT
(In reply to Alex Christensen from comment #6)
> Comment on attachment 407288 [details]
> Patch
> 
> Ah, ok.  Please try to remove any identical files, at least.

I removed some of them from glib.

Lauro, hopefully this will not cause regression on your side.
There might be other glib specific -expected.txt files that could be removed after this patch.
Comment 9 EWS 2020-08-27 03:44:31 PDT
Committed r266230: <https://trac.webkit.org/changeset/266230>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407387 [details].
Comment 10 Radar WebKit Bug Importer 2020-08-27 03:45:25 PDT
<rdar://problem/67864206>
Comment 11 Lauro Moura 2020-08-27 20:58:55 PDT
(In reply to youenn fablet from comment #8)
> (In reply to Alex Christensen from comment #6)
> > Comment on attachment 407288 [details]
> > Patch
> > 
> > Ah, ok.  Please try to remove any identical files, at least.
> 
> I removed some of them from glib.
> 
> Lauro, hopefully this will not cause regression on your side.
> There might be other glib specific -expected.txt files that could be removed
> after this patch.

Sorry for not responding earlier.

We're still getting some glib/soup-specific messages, like:

--- /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/http/tests/websocket/tests/hybi/long-control-frame-expected.txt
+++ /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/http/tests/websocket/tests/hybi/long-control-frame-actual.txt
@@ -1,4 +1,3 @@
-CONSOLE MESSAGE: WebSocket connection to 'ws://localhost:8880/websocket/tests/hybi/long-control-frame' failed: Received invalid WebSocket response from the server
 Test whether WebSocket rejects control frames longer than 125 bytes.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


I'll add back the glib baselines where only the error messages differ (i.e. no PASS/FAIL diffs against the baseline)

Regading GTK EWS, there is layout test builder in EWS-UAT: https://ews-build.webkit-uat.org/#/builders/34

We're working on adding more workers and stabilizing the flakies so it can be moved to the main queue.
Comment 12 Lauro Moura 2020-08-27 21:19:53 PDT
(In reply to Lauro Moura from comment #11)
> (In reply to youenn fablet from comment #8)
> > (In reply to Alex Christensen from comment #6)
> > > Comment on attachment 407288 [details]
> > > Patch
> > > 
> > > Ah, ok.  Please try to remove any identical files, at least.
> > 
> > I removed some of them from glib.
> > 
> > Lauro, hopefully this will not cause regression on your side.
> > There might be other glib specific -expected.txt files that could be removed
> > after this patch.
> 
> Sorry for not responding earlier.
> 
> We're still getting some glib/soup-specific messages, like:
> 
> ---
> /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/
> http/tests/websocket/tests/hybi/long-control-frame-expected.txt
> +++
> /home/buildbot/worker/gtk-linux-64-release-tests/build/layout-test-results/
> http/tests/websocket/tests/hybi/long-control-frame-actual.txt
> @@ -1,4 +1,3 @@
> -CONSOLE MESSAGE: WebSocket connection to
> 'ws://localhost:8880/websocket/tests/hybi/long-control-frame' failed:
> Received invalid WebSocket response from the server
>  Test whether WebSocket rejects control frames longer than 125 bytes.
>  
>  On success, you will see a series of "PASS" messages, followed by "TEST
> COMPLETE".
> 
> 
> I'll add back the glib baselines where only the error messages differ (i.e.
> no PASS/FAIL diffs against the baseline)
> 
> Regading GTK EWS, there is layout test builder in EWS-UAT:
> https://ews-build.webkit-uat.org/#/builders/34
> 
> We're working on adding more workers and stabilizing the flakies so it can
> be moved to the main queue.

Nevermind, I misread the diffs from the bots. I just need to update the rebaselines of failing tests removing the messages.
Comment 13 Lauro Moura 2020-08-27 21:49:51 PDT
Rebaselined the remaining glib files in r266274. Sorry for the noise.