Bug 64486 – WIN: NRWT has no fallback logic for WinPort

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

new-run-webkit-test's WinPort has no fallback logic

[Eric Seidel (no email)]

Created attachment 100714 [details]
Patch

[Eric Seidel (no email)]

Created attachment 100720 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 100720 [details]
Patch

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.)

[Eric Seidel (no email)]

I thought I had landed this.  Oops.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 100720 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100720&action=review

> > 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.

This was written this way because it was originally written to work with os.getwindowsversion() instead of our manual string parsing of ver.  I'm not sure it matters much.  tuples are easy to compare between.  But I guess strings are too?

> > 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?

Those are covered in a separate test.

> > 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

Will fix.

> > 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.)

Will fix.

[Eric Seidel (no email)]

Created attachment 103401 [details]
Updated patch, preparing to land

[Eric Seidel (no email)]

Committed r92717: <http://trac.webkit.org/changeset/92717>

[Eric Seidel (no email)]

This resulted in a test-webkitpy regression which is tracked by bug 66325.