Bug 54700

Created attachment 82903 [details] Patch

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

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

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

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