Bug 166660 – Add IOSDevice, share functionality with IOSSimulator in IOSBase

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Jonathan Bedard]

IOSDevice needs to be added.  IOSSimulator and IOSDevice should inherit from the shared class IOSBase.

[Jonathan Bedard]

Created attachment 297952 [details]
Patch

[Daniel Bates]

Comment on attachment 297952 [details]
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:179
> +    def extractBundleID(self, app):
> +        return self._executive.run_command(['/usr/libexec/PlistBuddy', '-c', '"Print CFBundleIdentifier"', 'Info.plist'], return_stderr=False)

This function is not used in this patch.

> Tools/Scripts/webkitpy/port/ios.py:44
> +class IOSBase(DarwinPort):

I suggest that we name this class IOSBasePort. This will make the name of this class consistent with the names of other port classes.

> Tools/Scripts/webkitpy/port/ios.py:49
> +        DarwinPort.__init__(self, host, port_name, **kwargs)

I know that you just moved this code. We should take this opportunity to modernize the code. In particular, we should write this line using super():

super(IOSBasePort, self).__init__(host, port_name, **kwargs)

The benefit of this approach is that if we will not need to update this code again should we change the base class for this derived class.

> Tools/Scripts/webkitpy/port/ios.py:51
> +        self._current_device = None

Currently this instance variable is specific to usage by the iOS Simulator port. It will likely turn out that we will need to know the current device for on-device testing. Having said that, I suggest that we defer making this change at this time and keep the patch focused on moving code from the IOSSimulatorPort class to this base class.

> Tools/Scripts/webkitpy/port/ios.py:95
> +    def _get_crash_log(self, name, pid, stdout, stderr, newer_than, time_fn=time.time, sleep_fn=time.sleep, wait_for_log=True):
> +        time_fn = time_fn or time.time
> +        sleep_fn = sleep_fn or time.sleep
> +
> +        # FIXME: We should collect the actual crash log for DumpRenderTree.app because it includes more
> +        # information (e.g. exception codes) than is available in the stack trace written to standard error.
> +        stderr_lines = []
> +        crashed_subprocess_name_and_pid = None  # e.g. ('DumpRenderTree.app', 1234)
> +        for line in (stderr or '').splitlines():
> +            if not crashed_subprocess_name_and_pid:
> +                match = self.SUBPROCESS_CRASH_REGEX.match(line)
> +                if match:
> +                    crashed_subprocess_name_and_pid = (match.group('subprocess_name'), int(match.group('subprocess_pid')))
> +                    continue
> +            stderr_lines.append(line)
> +
> +        if crashed_subprocess_name_and_pid:
> +            return self._get_crash_log(crashed_subprocess_name_and_pid[0], crashed_subprocess_name_and_pid[1], stdout,
> +                '\n'.join(stderr_lines), newer_than, time_fn, sleep_fn, wait_for_log)
> +
> +        _log.debug('looking for crash log for %s:%s' % (name, str(pid)))
> +        crash_log = ''
> +        crash_logs = CrashLogs(self.host)
> +        now = time_fn()
> +        deadline = now + 5 * int(self.get_option('child_processes', 1))
> +        while not crash_log and now <= deadline:
> +            crash_log = crash_logs.find_newest_log(name, pid, include_errors=True, newer_than=newer_than)
> +            if not wait_for_log:
> +                break
> +            if not crash_log or not [line for line in crash_log.splitlines() if not line.startswith('ERROR')]:
> +                sleep_fn(0.1)
> +                now = time_fn()
> +
> +        if not crash_log:
> +            return stderr, None
> +        return stderr, crash_log

Will this work for device without modification? If not, then I suggest we move this code back to class IOSSimulatorPort.

> Tools/Scripts/webkitpy/port/ios.py:109
> +    def _using_dedicated_simulators(self):
> +        return self.get_option('dedicated_simulators')
> +
> +    def _testing_device(self, number):
> +        return None
> +
> +    # This is only exposed so that IOSSimulatorDriver can use it.
> +    def device_id_for_worker_number(self, number):
> +        if self._printing_cmd_line:
> +            return '<dummy id>'
> +        if self._using_dedicated_simulators():
> +            return self._testing_device(number).udid
> +        return self._current_device.udid

Did you intend to move this? I mean, this function seems specific to the iOS Simulator port.

> Tools/Scripts/webkitpy/port/ios.py:112
> +class IOSDevice(IOSBase):

I suggest we call this class IOSDevicePort by a similar argument to the one I made for renaming IOSBase to IOSBasePort.

> Tools/Scripts/webkitpy/port/ios.py:167
> -        DarwinPort.__init__(self, host, port_name, **kwargs)
> +        IOSBase.__init__(self, host, port_name, **kwargs)

Similar to my remark in IOSBase.__init__() we should update this code to read:

super(IOSSimulatorPort, self).__init__(host, port_name, **kwargs)

[Jonathan Bedard]

Comment on attachment 297952 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/darwin.py:179
>> +        return self._executive.run_command(['/usr/libexec/PlistBuddy', '-c', '"Print CFBundleIdentifier"', 'Info.plist'], return_stderr=False)
> 
> This function is not used in this patch.

It will be used in another patch which is dependent on this patch.  Should it be moved to that patch instead?

>> Tools/Scripts/webkitpy/port/ios.py:51
>> +        self._current_device = None
> 
> Currently this instance variable is specific to usage by the iOS Simulator port. It will likely turn out that we will need to know the current device for on-device testing. Having said that, I suggest that we defer making this change at this time and keep the patch focused on moving code from the IOSSimulatorPort class to this base class.

On device testing will need to map devices to worker threads (see my comments on line 109 as well).  It's not clear exactly what this interface will look like, but it will likely be similar to the one used for simulators.  Given how high-level much of the device management is in the IOSSimulator class, I believe some of the same high-level logic can be shared between the two ports.

>> Tools/Scripts/webkitpy/port/ios.py:95
>> +        return stderr, crash_log
> 
> Will this work for device without modification? If not, then I suggest we move this code back to class IOSSimulatorPort.

I'm pretty sure the only modification needed will be to adjust where we're looking for crash logs.  Since this logic isn't part of the port classes anyways, I think this function makes more sense in the base class.  That being said, it's also possible that the crash logs don't get moved between device and host fast enough or predictably enough.  It's also possible we will need to include the device ID in some fashion since assuming one PID per crash won't work when dealing with multiple devices.

>> Tools/Scripts/webkitpy/port/ios.py:109
>> +        return self._current_device.udid
> 
> Did you intend to move this? I mean, this function seems specific to the iOS Simulator port.

The concept of a worker thread mapping to a device ID definitely applies to on device testing.  The '_using_dedicated_simulators()' flag is an idea specific to simulators, although it is a globally defined command line argument.

[Jonathan Bedard]

Created attachment 298014 [details]
Patch

[Jonathan Bedard]

Comment on attachment 298014 [details]
Patch

Delaying this patch till later.

[Jonathan Bedard]

This change is very much obsolete, especially with the addition of watchOS. Closing this bug.