Bug 182360

Summary: REGRESSION (r217572): run-webkit-tests exits without emitting newline character
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, Basuke.Suzuki, ews-watchlist, glenn, jbedard, lforschler, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar, Regression
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=212622
Bug Depends on: 172176    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Daniel Bates 2018-01-31 16:31:05 PST
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.
Comment 1 Daniel Bates 2018-01-31 16:35:20 PST
Created attachment 332821 [details]
Patch
Comment 2 Jonathan Bedard 2018-01-31 17:32:53 PST
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.
Comment 3 Daniel Bates 2018-01-31 22:43:36 PST
(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.
Comment 4 Daniel Bates 2018-01-31 22:46:58 PST
Created attachment 332849 [details]
Patch
Comment 5 Jonathan Bedard 2018-02-01 08:23:40 PST
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.
Comment 6 Daniel Bates 2018-02-01 09:07:32 PST
(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.
Comment 7 Daniel Bates 2018-02-01 09:08:31 PST
(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
Comment 8 Jonathan Bedard 2018-02-01 09:39:59 PST
(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.

> ...
Comment 9 Daniel Bates 2018-02-01 09:57:32 PST
(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.
Comment 10 Jonathan Bedard 2018-02-05 14:53:59 PST
(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.
Comment 11 Aakash Jain 2018-02-05 14:54:29 PST
rs=me
Comment 12 Daniel Bates 2018-02-05 15:11:08 PST
Committed r228133: <https://trac.webkit.org/changeset/228133>
Comment 13 Radar WebKit Bug Importer 2018-02-05 15:12:58 PST
<rdar://problem/37251751>