Bug 62615

Summary: nrwt: should clean up stale server processes from a previous run
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, ojan, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Bug Depends on: 62180    
Bug Blocks: 59993    
Attachments:
Description Flags
Patch
none
add better verification that stale processes are being killed per review feedback from tony tony: review+

Description Dirk Pranke 2011-06-13 19:14:03 PDT
nrwt: should clean up stale server processes from a previous run
Comment 1 Dirk Pranke 2011-06-13 19:39:32 PDT
Created attachment 97058 [details]
Patch
Comment 2 Tony Chang 2011-06-14 09:54:41 PDT
Comment on attachment 97058 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:181
> +            port._http_server = None
> +            port.start_http_server()

How do we verify that this worked?
Comment 3 Dirk Pranke 2011-06-14 10:40:10 PDT
Comment on attachment 97058 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:181
>> +            port.start_http_server()
> 
> How do we verify that this worked?

"working" here is defined as not raising an exception. If the first start() worked, there's a server running. If the second start() gets called, it'll check that it can bind to port 8000, and raise an error if it can't (see lines 170, 171, above for that test). In this case, it thinks the previous server is now stale, and kills it.

If I wanted to be more thorough, I could do any of the following things: verify that the first server was in fact killed, verify that a second process was running, or verify that we can connect to the port. None of those seemed strictly necessary given the other tests which ensure that things basically work, but I can add them if you'd prefer.

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:183
> +            port.stop_websocket_server()

This, on the other hand, is just a typo, and should be stop_http_server(), instead, obviously.
Comment 4 Tony Chang 2011-06-14 10:52:48 PDT
Comment on attachment 97058 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:181

>> 
>> How do we verify that this worked?
> 
> "working" here is defined as not raising an exception. If the first start() worked, there's a server running. If the second start() gets called, it'll check that it can bind to port 8000, and raise an error if it can't (see lines 170, 171, above for that test). In this case, it thinks the previous server is now stale, and kills it.
> 
> If I wanted to be more thorough, I could do any of the following things: verify that the first server was in fact killed, verify that a second process was running, or verify that we can connect to the port. None of those seemed strictly necessary given the other tests which ensure that things basically work, but I can add them if you'd prefer.

Verifying that we can connect to the port seems sufficient and not a lot of work.
Comment 5 Dirk Pranke 2011-06-14 13:12:27 PDT
Created attachment 97160 [details]
add better verification that stale processes are being killed per review feedback from tony
Comment 6 Dirk Pranke 2011-06-14 13:13:24 PDT
(In reply to comment #4)
> (From update of attachment 97058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97058&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:181
> 
> >> 
> >> How do we verify that this worked?
> > 
> > "working" here is defined as not raising an exception. If the first start() worked, there's a server running. If the second start() gets called, it'll check that it can bind to port 8000, and raise an error if it can't (see lines 170, 171, above for that test). In this case, it thinks the previous server is now stale, and kills it.
> > 
> > If I wanted to be more thorough, I could do any of the following things: verify that the first server was in fact killed, verify that a second process was running, or verify that we can connect to the port. None of those seemed strictly necessary given the other tests which ensure that things basically work, but I can add them if you'd prefer.
> 
> Verifying that we can connect to the port seems sufficient and not a lot of work.

Done. I added a check to make sure first process was in fact being killed, and revamped the test slightly to use two ports instead of resetting the state on the existing port.
Comment 7 Dirk Pranke 2011-06-15 19:47:59 PDT
Committed r88995: <http://trac.webkit.org/changeset/88995>