Bug 172176 – webkitpy: Start servers before setting-up for testing

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

On-device testing requires that servers are already running when devices are set-up.

[Radar WebKit Bug Importer]

<rdar://problem/32225538>

[Jonathan Bedard]

Created attachment 310275 [details]
Patch

[Dean Johnson]

Comment on attachment 310275 [details]
Patch

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

Unofficial r+.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:238
> +        needs_websockets = any(self._is_websocket_test(test) for test in tests_to_run)

I really like the use of `any` here, as well as the fact you used generators which means we get the best performance possible (other than doing all of this in a single loop). I also prefer this approach due to the readability and conciseness it provides to anyone reading!

[Jonathan Bedard]

Comment on attachment 310275 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:238
>> +        needs_websockets = any(self._is_websocket_test(test) for test in tests_to_run)
> 
> I really like the use of `any` here, as well as the fact you used generators which means we get the best performance possible (other than doing all of this in a single loop). I also prefer this approach due to the readability and conciseness it provides to anyone reading!

Glad you like it, but I can't take credit for it.

This was moved from def _run_tests, which was committee here: <https://trac.webkit.org/changeset/136730/webkit>

[WebKit Commit Bot]

Comment on attachment 310275 [details]
Patch

Clearing flags on attachment: 310275

Committed r216955: <http://trac.webkit.org/changeset/216955>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r216955 for reason:

This change caused strange behavior with web-platform-tests locally and on EWS.

Committed r217003: <http://trac.webkit.org/changeset/217003>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #7)
> Reverted r216955 for reason:
> 
> This change caused strange behavior with web-platform-tests locally and on
> EWS.
> 
> Committed r217003: <http://trac.webkit.org/changeset/217003>

In this bug, ios-sim EWS passed, but after the change was landed the tests failed on the trunk ios-sim testers: https://bugs.webkit.org/show_bug.cgi?id=172196

Chris was seeing this locally as well. He made a change that should have caused an existing WPT test to fail and it did fail when it was first run. When the test was retried, it passed.

[Jonathan Bedard]

I think this should be pretty easy to fix.

The issue is that we shut down the servers before between our main test run and retrying, which we should not do.

There is a larger issue, however, which is unrelated to either this change or Chris's change: we treat missing files as if they have passed.

[Jonathan Bedard]

Created attachment 310453 [details]
Patch

[Jonathan Bedard]

attachment 310453 [details] should fix the issues Ryan and Chris are talking about.  However, I'd like to mark tests as failing if we cannot access testing assets before letting this land, so that these failures do not occur silently.

[Jonathan Bedard]

Comment on attachment 310453 [details]
Patch

Was able to confirm that this will fix the problems Chris and Ryan were having.  The servers are not shut-down until retries are finished.

[Jonathan Bedard]

https://bugs.webkit.org/show_bug.cgi?id=172322 will cause failures like the ones Chris and Ryan were seeing to be logged in the future.

[Jonathan Bedard]

Created attachment 310678 [details]
Patch

[Jonathan Bedard]

Created attachment 311520 [details]
Patch

[Dean Johnson]

Comment on attachment 311520 [details]
Patch

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

> Tools/Scripts/webkitpy/port/base.py:980
> +        return bool(self._web_platform_test_server)

We should do our best to check the system to see if the WPT server is running, as compared to relying on Python's object state. We want to absolutely ensure there is no way these return invalid information given how these servers are used, and the bug with servers not running resulting in all passing tests (not sure if that one was fixed yet).

Can you find a more concrete way to see if the server is running on the system?

[Jonathan Bedard]

Created attachment 311523 [details]
Patch

[Jonathan Bedard]

(In reply to Dean Johnson from comment #16)
> Comment on attachment 311520 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=311520&action=review
> 
> > Tools/Scripts/webkitpy/port/base.py:980
> > +        return bool(self._web_platform_test_server)
> 
> We should do our best to check the system to see if the WPT server is
> running, as compared to relying on Python's object state. We want to
> absolutely ensure there is no way these return invalid information given how
> these servers are used, and the bug with servers not running resulting in
> all passing tests (not sure if that one was fixed yet).
> 
> Can you find a more concrete way to see if the server is running on the
> system?

I was actually checking web-platform sockets twice.  I needed to check the web socket server.  attachment 311523 [details] should work correctly.

[Jonathan Bedard]

Created attachment 311526 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311526 [details]
Patch

Clearing flags on attachment: 311526

Committed r217572: <http://trac.webkit.org/changeset/217572>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

This patch (In reply to WebKit Commit Bot from comment #20)
> Comment on attachment 311526 [details]
> Patch
> 
> Clearing flags on attachment: 311526
> 
> Committed r217572: <http://trac.webkit.org/changeset/217572>

This change does more than start servers earlier when running run-webkit-tests. Following this change run-webkit-tests no longer tries to start up servers if servers are already running on the machine. This has the side benefit that it is possible to invoke run-webkit-tests while run-webkit-httpd is running and run-webkit-tests will use the servers started by run-webkit-httpd. Prior to this change, run-webkit-tests would stop the HTTP server started by run-webkit-httpd and die with an exception when trying to start the web socket servers due to the ports being in use.

[Daniel Bates]

(In reply to WebKit Commit Bot from comment #20)
> Comment on attachment 311526 [details]
> Patch
> 
> Clearing flags on attachment: 311526
> 
> Committed r217572: <http://trac.webkit.org/changeset/217572>

This change caused run-webkit-tests to stop all servers twice and exit without printing a newline character following the printing of a log message telling a person that it was stopping the last server. See bug #182360.

[Carlos Alberto Lopez Perez]

(In reply to Daniel Bates from comment #22)
> This patch (In reply to WebKit Commit Bot from comment #20)
> > Comment on attachment 311526 [details]
> > Patch
> > 
> > Clearing flags on attachment: 311526
> > 
> > Committed r217572: <http://trac.webkit.org/changeset/217572>
> 
> This change does more than start servers earlier when running
> run-webkit-tests. Following this change run-webkit-tests no longer tries to
> start up servers if servers are already running on the machine. This has the
> side benefit that it is possible to invoke run-webkit-tests while
> run-webkit-httpd is running and run-webkit-tests will use the servers
> started by run-webkit-httpd. Prior to this change, run-webkit-tests would
> stop the HTTP server started by run-webkit-httpd and die with an exception
> when trying to start the web socket servers due to the ports being in use.

I think this new behavior is problematic and can cause hard-to-diagnose problems, for example if the user is running other services than the webkit-test-servers on this ports. I have opened bug 212622 for trying to fix this.