Bug 62615

Created attachment 97058 [details] Patch

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

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

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

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