The --runtime flag only works for iOS simulator. Instead, we should pass and iOS version, which would be meaningful on both simulator and device.
<rdar://problem/32952164>
Created attachment 314772 [details] Patch
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
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.
Created attachment 315851 [details] Patch
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.
Created attachment 315923 [details] Patch
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
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
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.
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.
Created attachment 316376 [details] Patch for landing
Comment on attachment 316376 [details] Patch for landing Clearing flags on attachment: 316376 Committed r219875: <http://trac.webkit.org/changeset/219875>
All reviewed patches have been landed. Closing bug.