[webkit-reviews] review granted: [Bug 183681] webkitpy: Unrecognized mac versions always use WebKitTestRunner : [Attachment 335972] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 183681: webkitpy: Unrecognized mac versions always use WebKitTestRunner
https://bugs.webkit.org/show_bug.cgi?id=183681

Attachment 335972: Patch

https://bugs.webkit.org/attachment.cgi?id=335972&action=review




--- Comment #9 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 335972
  --> https://bugs.webkit.org/attachment.cgi?id=335972
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.