Bug 189702 - webkitpy: Clobbering and building occurs multiple times for iOS Simulator ports
Summary: webkitpy: Clobbering and building occurs multiple times for iOS Simulator ports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-18 10:28 PDT by Jonathan Bedard
Modified: 2018-09-27 08:46 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.89 KB, patch)
2018-09-18 10:29 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2018-09-18 10:28:05 PDT
Currently, we clobber old results and and build in the _set_up_run function, which is run multiple times per run for the iOS simulator.
Comment 1 Jonathan Bedard 2018-09-18 10:28:16 PDT
<rdar://problem/44541704>
Comment 2 Jonathan Bedard 2018-09-18 10:29:34 PDT
Created attachment 350031 [details]
Patch
Comment 3 Jonathan Bedard 2018-09-18 10:43:33 PDT
Waiting for EWS since my local testing was limited.
Comment 4 WebKit Commit Bot 2018-09-18 13:10:41 PDT
Comment on attachment 350031 [details]
Patch

Clearing flags on attachment: 350031

Committed r236151: <https://trac.webkit.org/changeset/236151>
Comment 5 WebKit Commit Bot 2018-09-18 13:10:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2018-09-18 13:11:40 PDT
<rdar://problem/44572385>
Comment 7 Daniel Bates 2018-09-22 20:06:06 PDT
Comment on attachment 350031 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-167
> -        self._printer.write_update("Checking build ...")
> -        if not self._port.check_build():
> -            _log.error("Build check failed")
> -            return False
> -

Does run-webkit-tests on Mac run the tests if you perform a clean build of the WebKit stack and not the tools? This removal does not seem correct or does not seem correct for Mac. What guarantees that LayoutTestHelper was built before we try to launch it below? Even if we are guaranteed it to be built in advance it seems unintuitive that we moved this logic from this function, used to setup a run, to the function that is responsible for running the test. Maybe we have to do this for iOS, but then it may be better to make iOS the exception that it is than to modify all ports to fit iOS's weird model of the world.
Comment 8 Daniel Bates 2018-09-22 20:08:56 PDT
Comment on attachment 350031 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:240
> +        if self._options.clobber_old_results:
> +            self._clobber_old_results()
> +
> +        # Create the output directory if it doesn't already exist.
> +        self._port.host.filesystem.maybe_make_directory(self._results_directory)

Similarly this seems unintuitive. If this is necessary to fix iOS then make iOS the exception (and have dedicated conditional branches or a dedicated code path) and not make a mess of the code for all the other ports.
Comment 9 Jonathan Bedard 2018-09-27 08:46:29 PDT
Comment on attachment 350031 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-167
>> -
> 
> Does run-webkit-tests on Mac run the tests if you perform a clean build of the WebKit stack and not the tools? This removal does not seem correct or does not seem correct for Mac. What guarantees that LayoutTestHelper was built before we try to launch it below? Even if we are guaranteed it to be built in advance it seems unintuitive that we moved this logic from this function, used to setup a run, to the function that is responsible for running the test. Maybe we have to do this for iOS, but then it may be better to make iOS the exception that it is than to modify all ports to fit iOS's weird model of the world.

This function starts with an _. That indicates that it should be a private function and other classes shouldn't be calling it directly. This code wasn't removed, it was MOVED. We still run it. Just not multiple times.

All this change does is move this code so we don't do this multiple times during a single instantiation of run-webkit-tests for iOS Simulator. Since we've already established long ago that a single instantiation of run-webkit-tests may contain multiple sequential runs, it doesn't make sense for this code to be in _set_up_run(...), we only need it to be run once per instantiation of run-webkit-test.

It's also not just an iOS problem anymore because watchOS uses exactly the same model as iOS.