[webkit-reviews] review granted: [Bug 64486] new-run-webkit-test's WinPort has no fallback logic : [Attachment 100720] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 64486: new-run-webkit-test's WinPort has no fallback logic
https://bugs.webkit.org/show_bug.cgi?id=64486

Attachment 100720: Patch
https://bugs.webkit.org/attachment.cgi?id=100720&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100720&action=review


> Tools/ChangeLog:10
> +	   I've tried to write a patch for bug 64439 twice now, and both times
> +	   I've ended up re-writing half the port system.  So I'm breaking
> +	   things up into smaller pieces, this being the first.

Yay!

> Tools/Scripts/webkitpy/layout_tests/port/win.py:54
> +    def _version_string_from_windows_version_tuple(self,
windows_version_tuple):
> +	   if windows_version_tuple[:3] == (6, 1, 7600):
> +	       return '7sp0'
> +	   if windows_version_tuple[:2] == (6, 0):
> +	       return 'vista'
> +	   if windows_version_tuple[:2] == (5, 1):
> +	       return 'xp'
> +	   return None

Maybe it would be clearer to use a dot-separated string instead of a tuple?
That would have the advantage of requiring fewer conversions between strings,
tuples, and lists.

> Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:52
> +	   def mock_run_command(cmd):
> +	       self.assertEquals(cmd, ['cmd', '/c', 'ver'])
> +	       return "6.1.7600"

Is it worth testing other version strings we recognize? Is it worth testing any
version strings we don't recognize?

> Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:61
> +	   # Failures log to the pyton error log, but we have no easy way to
capture/test that.

Typo: pyton

> Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py:74
> +	   self._assert_search_path(['win-xp', 'win-vista', 'win-7sp0', 'win',
'mac-snowleopard', 'mac'], 'xp')

I think these _assert_search_path calls would be a little more readable if the
order of arguments were flipped. (I.e., the version should come first.)