Bug 54700 - fix various bugs causing test-webkitpy to fail on win32
Summary: fix various bugs causing test-webkitpy to fail on win32
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 48728 54788
  Show dependency treegraph
 
Reported: 2011-02-17 18:00 PST by Dirk Pranke
Modified: 2011-02-20 21:08 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.94 KB, patch)
2011-02-17 19:26 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback, more testing (10.53 KB, patch)
2011-02-18 18:51 PST, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-17 18:00:07 PST
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 :)
Comment 1 Dirk Pranke 2011-02-17 19:26:33 PST
Created attachment 82903 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-02-17 19:35:21 PST
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.
Comment 3 Ojan Vafai 2011-02-17 20:44:13 PST
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
Comment 4 Dirk Pranke 2011-02-18 18:51:36 PST
Created attachment 83047 [details]
update w/ review feedback, more testing
Comment 5 Ojan Vafai 2011-02-20 16:27:31 PST
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
Comment 6 Dirk Pranke 2011-02-20 21:08:35 PST
(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 .