Bug 183681

<rdar://problem/38509162>

Created attachment 335896 [details] Patch

Comment on attachment 335896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335896&action=review > Tools/Scripts/webkitpy/port/apple.py:69 > port_name = cls.port_name + '-' + host.platform.os_version_name().lower().replace(' ', '') > elif host.platform.os_version_name(): > - port_name = cls.port_name + '-' + host.platform.os_version_name().lower().replace(' ', '') + '-wk2' > + port_name = cls.port_name + '-' + host.platform.os_version_name().lower().replace(' ', '') > else: > - port_name = cls.port_name + '-wk2' > + port_name = cls.port_name > elif getattr(options, 'webkit_test_runner', False) and '-wk2' not in port_name: r-, take port_name to be "mac-lion-wk2" and options.webkit_test_runner to be False. Then we will treat this platform as WebKit1. This disagrees with the comment above this code and the behavior before <https://trac.webkit.org/changeset/229116>. > Tools/Scripts/webkitpy/port/mac.py:60 > + if len(port_name.split('-')) > 1: Can we write a test for this change? Would it make sense to separate out this change into its own bug? > Tools/Scripts/webkitpy/port/mac_unittest.py:158 > + # 2 minor versions from the current version should always be a future version. > + # If it is not, this test will fail. > + future_version = Version.from_iterable(MacPort.CURRENT_VERSION) > + future_version.minor += 2 :( Would it make sense to define a Version.FUTURE for this instead of defining it here? > Tools/Scripts/webkitpy/port/mac_unittest.py:166 > + port = self.make_port(options=MockOptions(webkit_test_runner=True), os_version=future_version, port_name='mac') > + self.assertEqual(port.driver_name(), 'WebKitTestRunner') > + self.assertEqual(port.version_name(), None) > + > + port = self.make_port(options=MockOptions(webkit_test_runner=False), os_version=future_version, port_name='mac') > + self.assertEqual(port.driver_name(), 'DumpRenderTree') > + self.assertEqual(port.version_name(), None) Please add tests for mac-lion, mac-lion-wk2, mac-wk2.

Comment on attachment 335896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335896&action=review >> Tools/Scripts/webkitpy/port/apple.py:69 >> elif getattr(options, 'webkit_test_runner', False) and '-wk2' not in port_name: > > r-, take port_name to be "mac-lion-wk2" and options.webkit_test_runner to be False. Then we will treat this platform as WebKit1. This disagrees with the comment above this code and the behavior before <https://trac.webkit.org/changeset/229116>. After writing the tests for this, it's actually 'mac-wk2' not 'mac-lion-wk2' which causes the problem, but your point stands. >> Tools/Scripts/webkitpy/port/mac.py:60 >> + if len(port_name.split('-')) > 1: > > Can we write a test for this change? Would it make sense to separate out this change into its own bug? The change is tested by setting the port name to 'mac' instead of 'mac-wk2'. It's defiantly part of this bug since before the change in apple.py, there would have never been a way through the port factory to have a port_name with a '-' in it at this point in the code. >> Tools/Scripts/webkitpy/port/mac_unittest.py:158 >> + future_version.minor += 2 > > :( Would it make sense to define a Version.FUTURE for this instead of defining it here? I wouldn't want to define MacPort.FUTURE_VERSION because there isn't a good reason to use that for anything other than testing. I would be Ok defining MacTest.FUTURE_VERSION, to indicate that the constant should only be used when running tests.

Created attachment 335938 [details] Patch

Comment on attachment 335938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335938&action=review > Tools/Scripts/webkitpy/port/apple.py:68 > - port_name = cls.port_name + '-wk2' > + port_name = cls.port_name r-, this is not correct and still disagrees with the comment above this code as I said in comment #3. Take port_name to be "mac-wk2" and cls.port_name to be "mac" and assume there is no name for the os version the machine is using.

I've added the following tests: mac, unnamed version, webkit_test_runner = True => Use WebKitTestRunner mac, unnamed version, webkit_test_runner = False => Use DumpRenderTree mac-wk2, unnamed version, webkit_test_runner = False => Use WebKitTestRunner mac, named version, webkit_test_runner = True => Use WebKitTestRunner mac, named version, webkit_test_runner = False => Use DumpRenderTree mac-wk2, named version, webkit_test_runner = False => Use WebKitTestRunner This is meant to test these two statements: Port names not ending in -wk2 should use the webkit_test_runner option to determine whether they are using DumpRenderTree or WebKitTestRunner Port names ending in -wk2 will override the webkit_test_runner option and always use WebKitTestRunner The treatment of -wk2 should not change when the version name (or lack of one) changes.

Created attachment 335972 [details] Patch

Comment on attachment 335972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335972&action=review > Tools/Scripts/webkitpy/port/apple.py:58 > + version_name_addition = ('-' + host.platform.os_version_name().lower().replace(' ', '')) if host.platform.os_version_name() else '' This is OK as-is. I wish we could come up with a better name for this variable. Maybe os_version name? It is unnecessary to explicitly do an extra branch and this computation is not hot enough to be a concern for us. I would done the following: os_version_name = (host.platform.os_version_name() or '').lower().replace(' ', '') Then substitute os_version_name for version_name_addition in the code below. > Tools/Scripts/webkitpy/port/mac.py:61 > + if len(port_name.split('-')) > 1 and port_name.split('-')[1] != 'wk2': > self._os_version = version_name_map.from_name(port_name.split('-')[1])[1] It is excessive to split port_name up to three times in this function. We should split it once, save it in a local, and then reference the local. > Tools/Scripts/webkitpy/port/mac_unittest.py:47 > + # 2 minor versions from the current version should always be a future version. > + FUTURE_VERSION = Version.from_iterable(MacPort.CURRENT_VERSION) > + FUTURE_VERSION.minor += 2 I wish there was a better way to represent a future OS version. Maybe we'll figure it out in the future ;) > Tools/Scripts/webkitpy/port/mac_unittest.py:164 > + port = self.make_port(options=MockOptions(webkit_test_runner=False), os_version=MacTest.FUTURE_VERSION, os_name='mac', port_name='mac-wk2') We should add a test case for the tautology webkit_test_runner := True and port_name='mac-wk2' to ensure we use WebKitTestRunner. > Tools/Scripts/webkitpy/port/mac_unittest.py:170 > + port = self.make_port(options=MockOptions(webkit_test_runner=False), os_version=MacTest.FUTURE_VERSION, os_name='mac', port_name='mac') > + self.assertEqual(port.driver_name(), 'DumpRenderTree') > + self.assertEqual(port.version_name(), None) For your consideration I suggest that we move this subtest to be under the first subtest in this function so as to group the port_name := 'mac' cases together. > Tools/Scripts/webkitpy/port/mac_unittest.py:172 > + def test_mac_port_name_with_wk2(self): Can we come up with a name that is more consistent with the name of the function above, test_future_version? Maybe we need to rename test_future_version? > Tools/Scripts/webkitpy/port/mac_unittest.py:173 > + port = self.make_port(options=MockOptions(webkit_test_runner=False), port_name='mac-lion') Is this case an accurate depiction of something that can happen in real life? Shouldn't we explicitly pass os_version to be Lion's version number? If this test represents a real life scenario then please add a test case that checks webkit_test_runner := False, os_version to be "version for Lion", and port_name := 'mac-lion'. We should also add a test when webkit_test_runner := True and port_name='mac-lion' both with and without os_version specified (if applicable). > Tools/Scripts/webkitpy/port/mac_unittest.py:177 > + port = self.make_port(options=MockOptions(webkit_test_runner=False), port_name='mac-lion-wk2') Is this case an accurate depiction of something that can happen in real life? Shouldn't we explicitly pass os_version to be Lion's version number? If this test represents a real life scenario then please add a test case that checks webkit_test_runner := False, os_version to be "version for Lion", and port_name := 'mac-lion-wk2'. We should also add a test when webkit_test_runner := True and port_name='mac-lion-wk2' both with and without os_version specified (if applicable). > Tools/Scripts/webkitpy/port/mac_unittest.py:181 > + port = self.make_port(options=MockOptions(webkit_test_runner=False), port_name='mac') We should add a test when webkit_test_runner := True and port_name='mac'. > Tools/Scripts/webkitpy/port/mac_unittest.py:185 > + self.assertEqual(port.driver_name(), 'DumpRenderTree') > + > + port = self.make_port(options=MockOptions(webkit_test_runner=False), port_name='mac-wk2') > + self.assertEqual(port.driver_name(), 'WebKitTestRunner') Unlike all the other subtests, these subtests do not assert a value for port.version_name(). We should be consistent in our testing. We should add a test for the tautology when webkit_test_runner := True and port_name='mac-wk2' to ensure we use WebKitTestRunner.

Comment on attachment 335972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335972&action=review >> Tools/Scripts/webkitpy/port/mac_unittest.py:173 >> + port = self.make_port(options=MockOptions(webkit_test_runner=False), port_name='mac-lion') > > Is this case an accurate depiction of something that can happen in real life? Shouldn't we explicitly pass os_version to be Lion's version number? If this test represents a real life scenario then please add a test case that checks webkit_test_runner := False, os_version to be "version for Lion", and port_name := 'mac-lion'. > > We should also add a test when webkit_test_runner := True and port_name='mac-lion' both with and without os_version specified (if applicable). This would mean that the user is deliberately overriding the os version to change which test expectations are used. If a user where to use: 'run-webkit-tests --platform mac-lion,' run-webkit-tests would use Lion test expectations, even if the host system was not Lion.

Comment on attachment 335972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=335972&action=review >> Tools/Scripts/webkitpy/port/mac_unittest.py:185 >> + self.assertEqual(port.driver_name(), 'WebKitTestRunner') > > Unlike all the other subtests, these subtests do not assert a value for port.version_name(). We should be consistent in our testing. > > We should add a test for the tautology when webkit_test_runner := True and port_name='mac-wk2' to ensure we use WebKitTestRunner. Good point about not asserting the value for port.version_name(). I did this because when using 'mac' or 'mac-wk2,' the version will be whatever the system's version is. At the moment, this is hard-coded as Lion for testing, but I don't see a reason to introduce code that would break if this hard-coded version was updated. To retain consistency, I will separate this second testing function into two testing functions.

Created attachment 336418 [details] Patch

(In reply to Jonathan Bedard from comment #12) > Created attachment 336418 [details] > Patch Sending through EWS before landing.

Comment on attachment 336418 [details] Patch Attachment 336418 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7079788 New failing tests: media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-drag.html

Created attachment 336436 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 336447 [details] Patch

(In reply to Jonathan Bedard from comment #16) > Created attachment 336447 [details] > Patch Sending back through EWS, the mac test failure and windows build failure should have already been resolved in the tree.

Comment on attachment 336447 [details] Patch Clearing flags on attachment: 336447 Committed r229955: <https://trac.webkit.org/changeset/229955>

All reviewed patches have been landed. Closing bug.

<rdar://problem/38831482>