On-device testing requires that servers are already running when devices are set-up.
<rdar://problem/32225538>
Created attachment 310275 [details] Patch
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!
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>
Comment on attachment 310275 [details] Patch Clearing flags on attachment: 310275 Committed r216955: <http://trac.webkit.org/changeset/216955>
All reviewed patches have been landed. Closing bug.
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 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.
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.
Created attachment 310453 [details] Patch
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.
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.
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.
Created attachment 310678 [details] Patch
Created attachment 311520 [details] Patch
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?
Created attachment 311523 [details] Patch
(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.
Created attachment 311526 [details] Patch
Comment on attachment 311526 [details] Patch Clearing flags on attachment: 311526 Committed r217572: <http://trac.webkit.org/changeset/217572>
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.
(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.
(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.