Bug 199710 – Remove pywebsockets from thirdparty

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We have a very old version of pywebsocket in webkitpy thirdparty, but we are also importing pywebsocket as part of wpt tools. We can simply use the wpt one for all websocket tests and remove the old copy from sources.

[Carlos Garcia Campos]

Created attachment 373924 [details]
Patch

[Carlos Garcia Campos]

It seems to be failing to start the websockets sever in the bots, but there's no much info in the output. It's working for me locally...

[Ryan Haddad]

I released the patch from the unhappy EWS queues since it was causing them to retry in a loop.

[Carlos Garcia Campos]

Traceback (most recent call last):
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/standalone.py", line 177, in <module>
    from mod_pywebsocket import common
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/common.py", line 35, in <module>
    from mod_pywebsocket import http_header_util
  File "/Volumes/external/cgarcia/WebKit/LayoutTests/imported/w3c/web-platform-tests/tools/pywebsocket/mod_pywebsocket/http_header_util.py", line 256, in <module>
    urllib.parse.uses_netloc.index('ws')
AttributeError: 'Module_six_moves_urllib_parse' object has no attribute 'uses_netloc'

It seems mac has an old version of six. We need to ensure we use six from wpt tools thirdparty dir too.

[Carlos Garcia Campos]

Created attachment 374283 [details]
Patch

[EWS Watchlist]

Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12757338

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html

[EWS Watchlist]

Created attachment 374284 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

(In reply to Build Bot from comment #6)
> Comment on attachment 374283 [details]
> Patch
> 
> Attachment 374283 [details] did not pass mac-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12757338
> 
> New failing tests:
> http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-
> headers.html

Interesting, it could be that this test depends on old impl of pywebsockets somehow. I'll investigate.

[EWS Watchlist]

Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12757348

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html

[EWS Watchlist]

Created attachment 374285 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 374283 [details]
Patch

Attachment 374283 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12757362

New failing tests:
http/tests/websocket/tests/hybi/handshake-ok-with-legacy-websocket-response-headers.html

[EWS Watchlist]

Created attachment 374289 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

The thing is that now the server is not sending the close message back after we close the connection in this test. So, we end up waiting for a long time in m_closingTimer (TCPMaximumSegmentLifetime * 2). I've tested it with chromium and the test takes 60 seconds to finish (which is the closing timer timeout used by chromium). I still don't know why this is happening with this test only.

[Carlos Garcia Campos]

Other tests using handshake.AbortedByUserException() are either expected failures or don't close the socket, just considering that onopen called is a success. We can do the same for this one.

[Carlos Garcia Campos]

Created attachment 374383 [details]
Patch

[EWS Watchlist]

Comment on attachment 374383 [details]
Patch

Attachment 374383 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12766458

New failing tests:
storage/indexeddb/dont-wedge.html

[EWS Watchlist]

Created attachment 374390 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Carlos Garcia Campos]

(In reply to Build Bot from comment #16)
> Comment on attachment 374383 [details]
> Patch
> 
> Attachment 374383 [details] did not pass mac-debug-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12766458
> 
> New failing tests:
> storage/indexeddb/dont-wedge.html

This is unrelated for sure.

[Jonathan Bedard]

Comment on attachment 374383 [details]
Patch

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

Python changes mostly looks good, I have one small question about six.

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:123
> +        pywebsocket_deps = [self._filesystem.join(wpt_tools_base, "third_party", "six")]

We have our own version of six in Tools/Scripts/webkitpy/thirdparty/autoinstalled. I don't think that we have ambiguity problems because it's always imported through 'import autoinstalled.six',  but this still seems potentially dangerous...especially if the wpt version of six is different than the one we have have previously loaded. Do we actually need this? We probably do, it just feels super weird to have six both autoinstalled and checked into LayoutTests/imported/w3c.

[Carlos Garcia Campos]

I think we have six in autoinstalled, because it's a dependency of another module that is autoinstalled (maybe pytest). wpt also includes six because it's a depedency of other wpt tools. So, in this case I'm using the six from wpt tools because I'm using another wpt tool. wpt tools include all tools in the sys path (using the localpaths module) and webkitpy uses the wk autoinstall module, so I don't think there's conflict.

[Jonathan Bedard]

Comment on attachment 374383 [details]
Patch

storage/indexeddb/dont-wedge.html is a flakey test at the moment, so I'd say this is good to land (See <https://bugs.webkit.org/show_bug.cgi?id=199883>)

[WebKit Commit Bot]

Comment on attachment 374383 [details]
Patch

Clearing flags on attachment: 374383

Committed r247640: <https://trac.webkit.org/changeset/247640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.