run-webkit-tests always exits without printing newline character: $ Tools/Scripts/run-webkit-tests Expected to fail, but passed: (7) ... Stopping WebSocket server ...$ This regressed with the patch for bug #172176.
Created attachment 332821 [details] Patch
Comment on attachment 332821 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332821&action=review It looks like this is two changes. One (as your ChangeLog indicates) will fix the logging bug caused by calling LayoutTestRunner.stop_servers(...) twice. The second change is caused by moving the web socket and wpt server checks from the port object to workspace.py. I think that this change makes sense as well, but it feels conceptually separate from the first change. > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:200 > + if self._needs_websockets: I'm pretty sure this is a change in behavior. This will mean that run-webkit-tests requires a web socket server, it will always boot one even if one is already booted. This is not called out in the ChangeLog, I'm not entirely sure that it's intentional. > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:203 > + if self._needs_web_platform_test_server: Ditto.
(In reply to Jonathan Bedard from comment #2) > The second change is caused by moving the web socket and wpt server checks > from the port object to workspace.py. I think that this change makes sense > as well, but it feels conceptually separate from the first change. > I no longer need this part. > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:200 > > + if self._needs_websockets: > > I'm pretty sure this is a change in behavior. This will mean that > run-webkit-tests requires a web socket server, it will always boot one even > if one is already booted. This is not called out in the ChangeLog, I'm not > entirely sure that it's intentional. > Will restore to allow a person to run run-webkit-tests while simultaneously running run-webkit-httpd.
Created attachment 332849 [details] Patch
Comment on attachment 332849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332849&action=review One minor comment, other than that, it looks good to me. > Tools/Scripts/webkitpy/port/base.py:978 > + return True What is the benefit of this? Shouldn't http_server_base.is_http_server_running() return true if self._http_server is defined anyways? I suppose you could have self._http_server defined but not started, although I think the only way that could happen is if a server through an exception while starting. > Tools/Scripts/webkitpy/port/base.py:984 > + return websocket_server.is_web_socket_server_running() Ditto. > Tools/Scripts/webkitpy/port/base.py:988 > + return True Ditto.
(In reply to Jonathan Bedard from comment #5) > Comment on attachment 332849 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332849&action=review > > One minor comment, other than that, it looks good to me. > > > Tools/Scripts/webkitpy/port/base.py:978 > > + return True > > What is the benefit of this? Shouldn't http_server_base.is_http_server_running() return true if self._http_server is defined anyways? Notice that http_server_base.is_http_server_running() is a free function that just turns around and calls the class method HttpServerBase._is_running_on_port(): <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py?rev=227745#L240>. This function does not know whether an HTTP server was started by webkitpy. > I suppose you could have self._http_server defined but > not started, although I think the only way that could happen is if a server > through an exception while starting. > > > Tools/Scripts/webkitpy/port/base.py:984 > > + return websocket_server.is_web_socket_server_running() > > Ditto. > Needed by a similar argument as above. > > Tools/Scripts/webkitpy/port/base.py:988 > > + return True > > Ditto. Needed by a similar argument as above.
(In reply to Daniel Bates from comment #6) > > What is the benefit of this? Shouldn't http_server_base.is_http_server_running() return true if self._http_server is defined anyways? > > [...] This function does not know whether an HTTP server was started by webkitpy. *run-webkit-tests
(In reply to Daniel Bates from comment #6) > (In reply to Jonathan Bedard from comment #5) > > ... > > > > What is the benefit of this? Shouldn't http_server_base.is_http_server_running() return true if self._http_server is defined anyways? > > Notice that http_server_base.is_http_server_running() is a free function > that just turns around and calls the class method > HttpServerBase._is_running_on_port(): > <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/ > servers/http_server_base.py?rev=227745#L240>. This function does not know > whether an HTTP server was started by webkitpy. I recognize that http_server_base.is_http_server_running() is actually checking if the expected port on the system is in use. What I'm pointing out is that http_server_base.is_http_server_running() is designed to detect when any server is running on the expected port, this should include one created and managed by webkitpy. The code proposed here will definitely work, but it seems redundant. Without the 'if' statement added here, this function should still return true if self._http_server.start() has been called. As mentioned previously, the only functional change here is that Port.is_http_server_running() will now return 'True' if self._http_server was instantiated but not started. > ...
(In reply to Jonathan Bedard from comment #8) > (In reply to Daniel Bates from comment #6) > > (In reply to Jonathan Bedard from comment #5) > > > ... > > > > > > What is the benefit of this? Shouldn't http_server_base.is_http_server_running() return true if self._http_server is defined anyways? > > > > Notice that http_server_base.is_http_server_running() is a free function > > that just turns around and calls the class method > > HttpServerBase._is_running_on_port(): > > <http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/ > > servers/http_server_base.py?rev=227745#L240>. This function does not know > > whether an HTTP server was started by webkitpy. > > I recognize that http_server_base.is_http_server_running() is actually > checking if the expected port on the system is in use. What I'm pointing out > is that http_server_base.is_http_server_running() is designed to detect when > any server is running on the expected port, this should include one created > and managed by webkitpy. The code proposed here will definitely work, but it > seems redundant. Without the 'if' statement added here, this function should > still return true if self._http_server.start() has been called. As mentioned > previously, the only functional change here is that > Port.is_http_server_running() will now return 'True' if self._http_server > was instantiated but not started. > > > ... It is unnecessary and excessive to allocate a socket and to connect to a server when the Port knows if it started a server or not based on the nullity of Port._http_server.
(In reply to Daniel Bates from comment #9) > (In reply to Jonathan Bedard from comment #8) > > ... > > It is unnecessary and excessive to allocate a socket and to connect to a > server when the Port knows if it started a server or not based on the > nullity of Port._http_server. Ok! Looks good to me then.
rs=me
Committed r228133: <https://trac.webkit.org/changeset/228133>
<rdar://problem/37251751>