Bug 173775 – Replace --runtime with something for both ios-simulator and ios-device

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

[Jonathan Bedard]

The --runtime flag only works for iOS simulator.  Instead, we should pass and iOS version, which would be meaningful on both simulator and device.

[Radar WebKit Bug Importer]

<rdar://problem/32952164>

[Jonathan Bedard]

Created attachment 314772 [details]
Patch

[Aakash Jain]

Comment on attachment 314772 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:298
>          optparse.make_option('--runtime', help='iOS Simulator runtime identifier (default: latest runtime)'),

do we want to allow passing both --runtime and --version? Is the plan to remove --runtime entirely?

> Tools/Scripts/webkitpy/port/ios.py:111
> +        if version_identifier:

you should return early if version_identifier is not specified, something like:

if not version_identifier:
    return None

> Tools/Scripts/webkitpy/port/ios.py:112
> +            split_by_period = version_identifier.split('.')

It would be a good idea to add a comment indicating sample version_identifier. It would make it easy for reader to understand why/how are we splitting and parsing the data.

> Tools/Scripts/webkitpy/port/ios.py:118
> +            return version_identifier

seems like all you are doing here is verifying whether version_identifier is valid or not. It would be a good idea to separate it out in another function, something like is_valid_ios_version()

> Tools/Scripts/webkitpy/port/ios_device.py:102
>          if len(self._device_for_worker_number_map()) == 0:

I know this is not part of this patch. But maybe you can simplify it to in this patch:

if not self._device_for_worker_number_map():

This would comply with webkit guideline: https://webkit.org/code-style-guidelines/#null-false-and-zero

[Jonathan Bedard]

Comment on attachment 314772 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:298
>>          optparse.make_option('--runtime', help='iOS Simulator runtime identifier (default: latest runtime)'),
> 
> do we want to allow passing both --runtime and --version? Is the plan to remove --runtime entirely?

I think we should continue to allow --runtime, although, I don't feel strongly about this.  We don't use --runtime anywhere in our automation.

[Jonathan Bedard]

Created attachment 315851 [details]
Patch

[Aakash Jain]

Comment on attachment 315851 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:109
> +    def _is_valid_ios_version(version_identifier):

It would be good if we can do this input validation at the time of parsing the arguments. That way we can confidently use self.get_option('version') anywhere in code without need for calling this method at multiple places.

> Tools/Scripts/webkitpy/port/ios_device.py:97
> +        if version_identifier and IOSPort._is_valid_ios_version(version_identifier):

I believe we can simplify this if else to:

if IOSPort._is_valid_ios_version(version_identifier):
    return version_identifier

> Tools/Scripts/webkitpy/port/ios_device.py:99
> +        elif version_identifier:

In general we should use 'if', instead of 'elif', when the previous 'if' has a return statement.
see: https://webkit.org/code-style-guidelines/#linebreaking-else-if

In this case, we should just remove this elif and raise the exception from inside _is_valid_ios_version()

> Tools/Scripts/webkitpy/port/ios_simulator.py:112
> +            raise RuntimeError('{} is an invalid iOS version'.format(version_identifier))

Same comment here about simplifying if elif.

[Jonathan Bedard]

Created attachment 315923 [details]
Patch

[Build Bot]

Comment on attachment 315923 [details]
Patch

Attachment 315923 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4148824

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 315941 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Aakash Jain]

Comment on attachment 315923 [details]
Patch

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

> Tools/Scripts/webkitpy/port/ios.py:119
> +        return True

we can simplify above 4 lines to something like:

return all(part.isdigit() for part in split_by_period)

> Tools/Scripts/webkitpy/port/ios.py:121
> +    def get_option(self, name, default_value=None):

This doesn't exactly validates the input at the time of parsing the arguments, but should be ok.

[Jonathan Bedard]

Comment on attachment 315923 [details]
Patch

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

>> Tools/Scripts/webkitpy/port/ios.py:121
>> +    def get_option(self, name, default_value=None):
> 
> This doesn't exactly validates the input at the time of parsing the arguments, but should be ok.

True, it is basically checked at access time.  This seems more consistent with the way we handle the runtime option.

[Jonathan Bedard]

Created attachment 316376 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 316376 [details]
Patch for landing

Clearing flags on attachment: 316376

Committed r219875: <http://trac.webkit.org/changeset/219875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.