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

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied 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 335896: Patch

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




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