Bug 183681 - webkitpy: Unrecognized mac versions always use WebKitTestRunner
Summary: webkitpy: Unrecognized mac versions always use WebKitTestRunner
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks: 185054
  Show dependency treegraph
 
Reported: 2018-03-15 15:22 PDT by Jonathan Bedard
Modified: 2018-04-26 16:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.30 KB, patch)
2018-03-15 15:39 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2018-03-16 08:42 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (6.86 KB, patch)
2018-03-16 14:41 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (8.13 KB, patch)
2018-03-23 13:38 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (2.94 MB, application/zip)
2018-03-23 16:04 PDT, EWS Watchlist
no flags Details
Patch (8.13 KB, patch)
2018-03-23 17:25 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2018-03-15 15:22:47 PDT
Unrecognized mac versions will always use the WebKit2 WebKitTestRunner, even if the user requests the WebKitLegacy DumpRenderTree test runner.
Comment 1 Jonathan Bedard 2018-03-15 15:25:39 PDT
<rdar://problem/38509162>
Comment 2 Jonathan Bedard 2018-03-15 15:39:20 PDT
Created attachment 335896 [details]
Patch
Comment 3 Daniel Bates 2018-03-15 20:14:57 PDT
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 4 Jonathan Bedard 2018-03-16 08:35:23 PDT
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.
Comment 5 Jonathan Bedard 2018-03-16 08:42:42 PDT
Created attachment 335938 [details]
Patch
Comment 6 Daniel Bates 2018-03-16 12:08:26 PDT
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.
Comment 7 Jonathan Bedard 2018-03-16 14:39:44 PDT
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.
Comment 8 Jonathan Bedard 2018-03-16 14:41:22 PDT
Created attachment 335972 [details]
Patch
Comment 9 Daniel Bates 2018-03-23 10:29:01 PDT
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 10 Jonathan Bedard 2018-03-23 13:24:39 PDT
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 11 Jonathan Bedard 2018-03-23 13:30:46 PDT
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.
Comment 12 Jonathan Bedard 2018-03-23 13:38:49 PDT
Created attachment 336418 [details]
Patch
Comment 13 Jonathan Bedard 2018-03-23 13:39:59 PDT
(In reply to Jonathan Bedard from comment #12)
> Created attachment 336418 [details]
> Patch

Sending through EWS before landing.
Comment 14 EWS Watchlist 2018-03-23 16:04:14 PDT
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
Comment 15 EWS Watchlist 2018-03-23 16:04:15 PDT
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
Comment 16 Jonathan Bedard 2018-03-23 17:25:50 PDT
Created attachment 336447 [details]
Patch
Comment 17 Jonathan Bedard 2018-03-23 17:26:49 PDT
(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 18 WebKit Commit Bot 2018-03-24 13:46:56 PDT
Comment on attachment 336447 [details]
Patch

Clearing flags on attachment: 336447

Committed r229955: <https://trac.webkit.org/changeset/229955>
Comment 19 WebKit Commit Bot 2018-03-24 13:46:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Radar WebKit Bug Importer 2018-03-24 13:47:20 PDT
<rdar://problem/38831482>