Summary: | Rebaseline server: list current baselines and platforms | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||
Component: | Tools / Tests | Assignee: | Mihai Parparita <mihaip> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | dpranke, tony | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | All | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 47761 | ||||||
Attachments: |
|
Description
Mihai Parparita
2010-11-23 14:03:03 PST
Created attachment 74695 [details]
Patch
Comment on attachment 74695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74695&action=review > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:65 > + return '/'.join(trimmed_comps) I guess unittests hard code the expectation of '/' instead of using os.sep? > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:93 > + platforms.platforms.forEach(function(platform) { > + var platformOption = document.createElement('option'); > + platformOption.value = platform; Nit: 4 space indent > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:202 > + for baseline_extension in ['.txt', '.checksum', '.png']: Nit: () instead of [] > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:65 > + 'base': ['.txt'], > + 'mac': ['.checksum', '.png'], > + 'win': ['.checksum', '.png'], () is normally preferred over [] (it's a tiny bit faster and enforces const-ness), but for single item lists, it's a bit more error prone since you need to remember the trailing comma. Anyway, either way is fine here. (In reply to comment #2) > (From update of attachment 74695 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74695&action=review > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:65 > > + return '/'.join(trimmed_comps) > > I guess unittests hard code the expectation of '/' instead of using os.sep? I guess so. Dirk can say for sure, MockFileSystem was recently added by him. > > WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:93 > > + platforms.platforms.forEach(function(platform) { > > + var platformOption = document.createElement('option'); > > + platformOption.value = platform; > > Nit: 4 space indent Fixed. > > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:202 > > + for baseline_extension in ['.txt', '.checksum', '.png']: > > Nit: () instead of [] Fixed. > > WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:65 > > + 'base': ['.txt'], > > + 'mac': ['.checksum', '.png'], > > + 'win': ['.checksum', '.png'], > > () is normally preferred over [] (it's a tiny bit faster and enforces const-ness), but for single item lists, it's a bit more error prone since you need to remember the trailing comma. Anyway, either way is fine here. Switched to (). Committed r72640: <http://trac.webkit.org/changeset/72640> (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 74695 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=74695&action=review > > > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:65 > > > + return '/'.join(trimmed_comps) > > > > I guess unittests hard code the expectation of '/' instead of using os.sep? > > I guess so. Dirk can say for sure, MockFileSystem was recently added by him. > I'm not quite sure which "unittests" are being referred to here, but filesystem_mock is intended to provide predictable deterministic output regardless of what platform you're actually running on. At some point I was thinking that this made writing tests easier because you could assume '/' in the output, as you say. Thinking about it now, though, it might've been better to expose a filesystem.sep and use something like "--SEPARATOR--" as the value provided by the Mock, just to catch people that were assuming "/" in their paths. Thoughts? -- Dirk (In reply to comment #5) > Thinking about it now, though, it might've been better to expose a filesystem.sep and use something like "--SEPARATOR--" as the value provided by the Mock, just to catch people that were assuming "/" in their paths. Sure, I like that plan. Might as well do it before too many usages of (Mock)Filesystem crop up. Want me to do it? (In reply to comment #6) > (In reply to comment #5) > > Thinking about it now, though, it might've been better to expose a filesystem.sep and use something like "--SEPARATOR--" as the value provided by the Mock, just to catch people that were assuming "/" in their paths. > > Sure, I like that plan. Might as well do it before too many usages of (Mock)Filesystem crop up. Want me to do it? Sure, if you like, that would be great. FWIW, I think it's always safe to use '/' as a path separator (works fine on Windows too). However if a test has a call to os.path.abspath or os.path.normpath, the test might have a different result on Windows. (In reply to comment #8) > FWIW, I think it's always safe to use '/' as a path separator (works fine on Windows too). However if a test has a call to os.path.abspath or os.path.normpath, the test might have a different result on Windows. I think it depends on the context. I've certainly seen bugs where I've called out to the shell and passed it filenames with a '/' and the binary wanted '\'. I think you need to be careful about in-cygwin vs. not-in-cygwin code paths and cygwin vs. win32 binaries. I think cygwin is supposed to convert paths in some cases but it doesn't/can't always do the right thing. (In reply to comment #6) > (In reply to comment #5) > > Thinking about it now, though, it might've been better to expose a filesystem.sep and use something like "--SEPARATOR--" as the value provided by the Mock, just to catch people that were assuming "/" in their paths. > > Sure, I like that plan. Might as well do it before too many usages of (Mock)Filesystem crop up. Want me to do it? Hm. I remember now. It is partially because it's a pain to create sample data for a mock filesystem otherwise. I make extensive use of the mock filesystem in the patch to bug 48072 (which hasn't landed yet). At that point, the filesystem wasn't really a mock anymore, and so I renamed it to in_memory_filesystem, where I felt comfortable using '/' as the separator. Given that, we can still make the --SEPARATOR-- change as described above, because it might still catch some bugs. The downside is that we might end up with more scaffolding code to maintain. Comment on attachment 74695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=74695&action=review >>>> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:65 >>>> + return '/'.join(trimmed_comps) >>> >>> I guess unittests hard code the expectation of '/' instead of using os.sep? >> >> I guess so. Dirk can say for sure, MockFileSystem was recently added by him. > > I'm not quite sure which "unittests" are being referred to here, but filesystem_mock is intended to provide predictable deterministic output regardless of what platform you're actually running on. At some point I was thinking that this made writing tests easier because you could assume '/' in the output, as you say. Thinking about it now, though, it might've been better to expose a filesystem.sep and use something like "--SEPARATOR--" as the value provided by the Mock, just to catch people that were assuming "/" in their paths. > > Thoughts? > > -- Dirk Thinking about this more, what was the code path that triggered the need for this change? The only place I see you doing this is in rebaseline_unittest:_assertBaselines, which doesn't look like the comps should have a trailing slash (I don't think layout_test_dir() returns something with a trailing slash). My concern is that this is possibly papering over incorrect usage of paths with hard-coded forward slashes. Did I miss something? (In reply to comment #11) > Thinking about this more, what was the code path that triggered the need for this change? > >The only place I see you doing this is in rebaseline_unittest:_assertBaselines, which doesn't look like the comps should have a trailing slash (I don't think layout_test_dir() returns something with a trailing slash). > > My concern is that this is possibly papering over incorrect usage of paths with hard-coded forward slashes. > > Did I miss something? Digging more into this, it's because config.webkit_base_dir returns a path with a trailing slash. We then join that with 'LayoutTests' in layout_tests_dir/path_from_webkit_base, so we would end up with path/to/webkitcheckout//layouttests. I can make config.webkit_base_dir chop off the trailing slash, since that seems more correct. (In reply to comment #12): > I can make config.webkit_base_dir chop off the trailing slash, since that seems more correct. Uploaded a patch to bug 50197 to make this change. |