Bug 182360 – REGRESSION (r217572): run-webkit-tests exits without emitting newline character

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 332821 [details]
Patch

[Jonathan Bedard]

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.

[Daniel Bates]

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

[Daniel Bates]

Created attachment 332849 [details]
Patch

[Jonathan Bedard]

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.

[Daniel Bates]

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

[Daniel Bates]

(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

[Jonathan Bedard]

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

> ...

[Daniel Bates]

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

[Jonathan Bedard]

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

[Aakash Jain]

rs=me

[Daniel Bates]

Committed r228133: <https://trac.webkit.org/changeset/228133>

[Radar WebKit Bug Importer]

<rdar://problem/37251751>