Bug 189702 – webkitpy: Clobbering and building occurs multiple times for iOS Simulator ports

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

[Jonathan Bedard]

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.

[Jonathan Bedard]

<rdar://problem/44541704>

[Jonathan Bedard]

Created attachment 350031 [details]
Patch

[Jonathan Bedard]

Waiting for EWS since my local testing was limited.

[WebKit Commit Bot]

Comment on attachment 350031 [details]
Patch

Clearing flags on attachment: 350031

Committed r236151: <https://trac.webkit.org/changeset/236151>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44572385>

[Daniel Bates]

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.

[Daniel Bates]

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.

[Jonathan Bedard]

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.