Bug 62615 – nrwt: should clean up stale server processes from a previous run

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: should clean up stale server processes from a previous run

[Dirk Pranke]

Created attachment 97058 [details]
Patch

[Tony Chang]

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?

[Dirk Pranke]

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.

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 97160 [details]
add better verification that stale processes are being killed per review feedback from tony

[Dirk Pranke]

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

[Dirk Pranke]

Committed r88995: <http://trac.webkit.org/changeset/88995>