Bug 54700 – fix various bugs causing test-webkitpy to fail on win32

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

There are various failures being caused by the way we're handling directory separators and mixing up the use of fake filesystems with real ones. Details in the ChangeLog :)

[Dirk Pranke]

Created attachment 82903 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 82903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82903&action=review

> Tools/Scripts/webkitpy/common/net/testoutput_unittest.py:70
>      def _check_name(self, filename, expected_test_name):
> +        filename = filename.replace('/', os.path.sep)
> +        expected_test_name = expected_test_name.replace('/', os.path.sep)
>          r = testoutput.TextTestOutput(None, FakeFile(filename))
>          self.assertEquals(expected_test_name, r.name())
>  
>      def _check_platform(self, filename, expected_platform):
> +        filename = filename.replace('/', os.path.sep)

Can/should we be fixing the paths at a higher level? It seems like ideally we'd construct them with the correct separator right from the start.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:120
> +    class TestWinPort(chromium_win.ChromiumWinPort):
> +        def __init__(self, options):
> +            chromium_win.ChromiumWinPort.__init__(self,
> +                                                  options=options,
> +                                                  filesystem=filesystem_mock.MockFileSystem())
> +
> +        def default_configuration(self):
> +            self.default_configuration_called = True
> +            return 'default'
> +

I don't see this mentioned in your ChangeLog.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:125
> -            '/out/default/ImageDiff'), msg=port._path_to_image_diff())
> +            'out/default/ImageDiff'))

I don't understand this change, but it's probably at least partly because I'm not familiar with this code.

[Ojan Vafai]

Comment on attachment 82903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82903&action=review

> Tools/Scripts/webkitpy/common/net/testoutput.py:51
> +        path = re.split(re.escape(os.path.sep), filename)

Can't you just do filename.split(os.path.sep)?

> Tools/Scripts/webkitpy/common/net/testoutput.py:57
> +        path = re.split(re.escape(os.path.sep), filename)

ditto

[Dirk Pranke]

Created attachment 83047 [details]
update w/ review feedback, more testing

[Ojan Vafai]

Comment on attachment 83047 [details]
update w/ review feedback, more testing

View in context: https://bugs.webkit.org/attachment.cgi?id=83047&action=review

> Tools/Scripts/webkitpy/common/net/testoutput_unittest.py:67
> +        filename = filename.replace('/', os.path.sep)
> +        expected_test_name = expected_test_name.replace('/', os.path.sep)

Maybe to be more clear we should only be doing this on non-cygwin windows? That's the only case where this does something, right?

> Tools/Scripts/webkitpy/common/net/testoutput_unittest.py:74
> +        filename = filename.replace('/', os.path.sep)

ditto

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 83047 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83047&action=review
> 
> > Tools/Scripts/webkitpy/common/net/testoutput_unittest.py:67
> > +        filename = filename.replace('/', os.path.sep)
> > +        expected_test_name = expected_test_name.replace('/', os.path.sep)
> 
> Maybe to be more clear we should only be doing this on non-cygwin windows? That's the only case where this does something, right?
> 
> > Tools/Scripts/webkitpy/common/net/testoutput_unittest.py:74
> > +        filename = filename.replace('/', os.path.sep)
> 
> ditto

Done, although I'm not sure that I think that's clearer, myself.

Committed in http://trac.webkit.org/changeset/79176 .