webkitpy: Efficient app installation for device testing

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Currently, any time a test causes the test-runner to crash, we re-install the app.  App installation should only occur once per device, and the user should be able to opt-out of running an app install.

[Radar WebKit Bug Importer]

<rdar://problem/30790207>

[Jonathan Bedard]

Created attachment 303111 [details]
Patch

[Jonathan Bedard]

Created attachment 303135 [details]
Patch

[Daniel Bates]

Comment on attachment 303135 [details]
Patch

How did you come to the decision to define a custom driver class, iOSDriver, to install the test tool app as opposed to having setup_test_run() install the test tool app after booting the simulator?

[Jonathan Bedard]

(In reply to comment #4)
> Comment on attachment 303135 [details]
> Patch
> 
> How did you come to the decision to define a custom driver class, iOSDriver,
> to install the test tool app as opposed to having setup_test_run() install
> the test tool app after booting the simulator?

2 reasons for this choice.

First, when we first started using simctl to install on simulated devices, we encountered some issues where the install would silently fail.  In order to detect this failure, we launch the app after install to check if the install was successful.  In-order for this to work, we need to pass the DYLD environment into the install.  The DYLD environment currently comes from the driver, not the port.  Second, and more importantly, if the install is a non-trivial task (as it would be on an actual device) placing this code in the driver will result in it being broken into the specific processes, as opposed to being run once during setup.  Right now, we assume install is a blocking action, so we cannot install to multiple devices at once from the same process.

The alternative you suggest would be to run all of the installs on one process before breaking up the test runs on multiple processes.  This would be slower, although it may also alleviate some of the flakiness we've observed with simctl install.

[Jonathan Bedard]

Created attachment 303519 [details]
Patch

[Daniel Bates]

Comment on attachment 303519 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:39
> +    # FIXME: The install_flag should not be needed if the driver is only created when running a test.
> +    def __init__(self, port, worker_number, pixel_tests, no_timeout=False, install_flag=False):
> +        super(iOSDriver, self).__init__(port, worker_number, pixel_tests, no_timeout, install_flag)
> +        if not install_flag:
> +            return

Adding a boolean flag, install_flag, is not the correct way to make this code work for the unit tests. Instantiating a driver should not have side-effects that make it infeasible to instantiate a driver just to query information and adding a boolean constructor argument to mitigate these side effects does not seem appropriate. Can we move this logic to Port.setup_test_run()? If we need to set the DYLD_FRAMEWORK_PATH before calling Device.install_app() then can we move _setup_environ_for_driver() to the base Port class and then have both the Driver and Port.setup_test_run() use it? Or, even better, can we write the workaround in Device.install_app() to detect successful installation without needing to launch the app? (Obviously, it would be best if we did not need to work around a CoreSimulator issue).

[Jonathan Bedard]

Created attachment 303545 [details]
Patch

[Daniel Bates]

Comment on attachment 303545 [details]
Patch

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

> Tools/Scripts/webkitpy/port/factory.py:52
>              help=('Alias for --platform=ios-simulator')),
> +        optparse.make_option('--no-install', action='store_const', dest='install',
> +            const=False, default=True,
> +            help='Skip app install for device testing'),

Is this necessity to do now? I mean, we do not have support for device testing.

> Tools/Scripts/webkitpy/port/ios.py:62
> +    def _check_driver(self):
> +        if self.get_option('install'):
> +            return super(IOSPort, self)._check_driver()
> +        return True
> +

Ditto.

> Tools/Scripts/webkitpy/port/ios.py:88
> +                _log.warn('Installing to {}'.format(self.device_for_worker_number(i)))

If we chose to log such details it seems most beneficial to debugging and hence I suggest we log the message to the debug log.

> Tools/Scripts/webkitpy/port/ios.py:89
> +                if not self.device_for_worker_number(i).install_app(self._path_to_driver(), {'DYLD_LIBRARY_PATH': self._build_path()}):

This seems to take advantage of knowledge of when DRT/WKTR make use of functionality from a built WebKit to avoid crashing. This seems error prone and would necessitate a comment to explain why we can get away with launching DRT/WKTR with only an overridden DYLD_LIBRARY_PATH as opposed to the full set of environment variables we set when running the tool for a normal test run. I gave a suggestion move the _setup_environ_for_driver() function to the base Port class in comment #7 as one way to share code between the port and driver classes and setup the environment.

> Tools/Scripts/webkitpy/port/ios.py:90
> +                    raise RuntimeError('Failed to install app {} on device {}'.format(self._path_to_driver(), self.device_for_worker_number(i).udid))

We compute self.device_for_worker_number(i) multiple times throughout this function. We should compute it once and cache it.

> Tools/Scripts/webkitpy/port/ios.py:91
> +                _log.warn('Install completed')

I do not see the need to log this information. I mean, its not a warning and if the install failed then we would have thrown a RuntimeError and never reached this line of code. In general, we follow the UNIX tool philosophy that a tool should be silent on success and noisy on failure. No need for a tool to make noise on success as we do here.

> Tools/Scripts/webkitpy/port/ios_simulator.py:224
> +            super(IOSSimulatorPort, self).setup_test_run(device_class)

This seems brittle. I mean, the logic for setting up a test for simulator is now split between this function and the base class function and requires a specific calling order (i.e. we cannot just call the base class function immediately at the start of this function). For on-device testing we will likely need similar logic to setup a run. One way to share code is to abstract the moving parts in this function (i.e. creating of simulator) into helper functions that must be implemented in the derived classes. The derived classes would not implement setup_test_run() instead the base class implements setup_test_run() and calls the helper functions in the derived class to perform the actual creation of a simulator/device setup.

[Jonathan Bedard]

Created attachment 303740 [details]
Patch

[Dean Johnson]

Comment on attachment 303740 [details]
Patch

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

> Tools/ChangeLog:10
> +        Move app installation to setup and add a command line flag to skip installing.

I don't see the new flag included in this patch? This comment should be removed or a flag should be added for --no-install.

[Jonathan Bedard]

Created attachment 303934 [details]
Patch

[Daniel Bates]

Comment on attachment 303934 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:92
> +            # Without passing DYLD_LIBRARY_PATH, libWebCoreTestSupport cannot be loaded and the app will crash pre-launch,
> +            # leaving a crash log which will be picked up in results.  No DYLD_FRAMEWORK_PATH will also cause the app to
> +            # crash, but this crash will occur post-launch, after install_app has already killed the process.
> +            if not device.install_app(self._path_to_driver(), {'DYLD_LIBRARY_PATH': self._build_path()}):

app => DRT/WKTR (as we are not talking about an arbitrary app here)

OK. This seems fragile. It would be less fragile to move the function that setups the environment variables from the driver code to the port class and use it from here and the driver code to setup the correct environment variables. I am hoping that we can remove the workaround in install_app() to launch the app and hence remove the need to pass any environment variables to it.

[Jonathan Bedard]

Created attachment 304403 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304403 [details]
Patch for landing

Clearing flags on attachment: 304403

Committed r213926: <http://trac.webkit.org/changeset/213926>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.