Bug 49991 – Rebaseline server: list current baselines and platforms

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Mihai Parparita]

Rebaseline server: list current baselines and platforms

[Mihai Parparita]

Created attachment 74695 [details]
Patch

[Tony Chang]

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.

[Mihai Parparita]

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

[Mihai Parparita]

Committed r72640: <http://trac.webkit.org/changeset/72640>

[Dirk Pranke]

(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

[Mihai Parparita]

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

[Dirk Pranke]

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

[Tony Chang]

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.

[Dirk Pranke]

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

[Dirk Pranke]

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

[Dirk Pranke]

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?

[Mihai Parparita]

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

[Mihai Parparita]

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