Bug 47319 – rebaseline-chromium-webkit-tests asserts when generating the html report

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

rebaseline-chromium-webkit-tests asserts when generating the html report

[Dirk Pranke]

Created attachment 70025 [details]
Patch

[Dirk Pranke]

note that I will also be using the new path.abspath_to_uri() code in the patch to fix 47220 (enabling new-run-webkit-tests to run under cygwin)

[Dirk Pranke]

Created attachment 70027 [details]
Patch

[Dirk Pranke]

move pulling of sys.platform from the default value of the argument into the function - the default value of the argument is evaluated when the function is defined, not invoked, making it impossible to be ovveridden in unit tests (noticed this when I tested the patch under cygwin).

[Adam Barth]

Comment on attachment 70027 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/common/system/path.py:1
> +#!/usr/bin/env python

We don't need these lines at the top of files.

> WebKitTools/Scripts/webkitpy/common/system/path.py:48
> +    return "file:///" + path.replace("\\", "/")

Do we need to do any URL escaping?  This feels like too many ///.  Do we want
file://c:/foo
or
file:///c:/foo
?

> WebKitTools/Scripts/webkitpy/common/system/path_unittest.py:34
> +class AbspathTest(unittest.TestCase):

Can you add some tests with %, space, +, and $ ?

> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:655
> +        return webkitpy.common.system.path.abspath_to_uri(path, self._executive)

Crazy.  I'd rather import this at the top of the file.  That makes it easier to grep the codebase and understand what's going on.

[Adam Barth]

Yay for deleting code.

[Dirk Pranke]

(In reply to comment #5)
> (From update of attachment 70027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70027&action=review
> 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:1
> > +#!/usr/bin/env python
> 
> We don't need these lines at the top of files.
>

True. I wonder how many of the files have them anyway. I'll take a pass through the code to look. 

AFAIK, we don't have an official style recommendation on this. Maybe we should? PEP 8 is silent, and the Google style guide recommends one (the AppEngine default, /usr/bin/python2.5, which doesn't necessarily work for us).
 
> > WebKitTools/Scripts/webkitpy/common/system/path.py:48
> > +    return "file:///" + path.replace("\\", "/")
> 
> Do we need to do any URL escaping?

Good question. This was the minimum amount of conversion we needed to do on any file that NRWT uses. As a generic routine it should probably be safer and do more.

>  This feels like too many ///.  Do we want
> file://c:/foo
> or
> file:///c:/foo
> ?
> 

file:///c:/foo . I just tested this to death as part of the fix for bug 47140, although looking back at the bug and the changelog comments, for some reason I failed to explicitly mention that. I'll go add a comment over there for the record.

> > WebKitTools/Scripts/webkitpy/common/system/path_unittest.py:34
> > +class AbspathTest(unittest.TestCase):
> 
> Can you add some tests with %, space, +, and $ ?
> 

Will do.

> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:655
> > +        return webkitpy.common.system.path.abspath_to_uri(path, self._executive)
> 
> Crazy.  I'd rather import this at the top of the file.  That makes it easier to grep the codebase and understand what's going on.

Yeah, I waffled on this a bit. There is an "import webkitpy.common.system.path" at the top of the file; it wouldn't work otherwise. So, I'm not entirely sure what grep you'd like to do that would fail?

Otherwise, I agree that this looks a bit weird. I don't normally like to import bare names from other modules, so I would've wanted something like "path.abspath_to_uri()", but I was worried that that would get confused with os.path and other things. Of course, adding more than one module to the name would be weirder, so I thought that maybe the full module path was least weird. I could easily be wrong. 

Would you prefer "path.abspath_to_uri" or "abspath_to_uri"?

[Adam Barth]

Comment on attachment 70027 [details]
Patch

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

>>> WebKitTools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:655
>>> +        return webkitpy.common.system.path.abspath_to_uri(path, self._executive)
>> 
>> Crazy.  I'd rather import this at the top of the file.  That makes it easier to grep the codebase and understand what's going on.
> 
> Yeah, I waffled on this a bit. There is an "import webkitpy.common.system.path" at the top of the file; it wouldn't work otherwise. So, I'm not entirely sure what grep you'd like to do that would fail?
> 
> Otherwise, I agree that this looks a bit weird. I don't normally like to import bare names from other modules, so I would've wanted something like "path.abspath_to_uri()", but I was worried that that would get confused with os.path and other things. Of course, adding more than one module to the name would be weirder, so I thought that maybe the full module path was least weird. I could easily be wrong. 
> 
> Would you prefer "path.abspath_to_uri" or "abspath_to_uri"?

Oh, you could do something like

import webkitpy.common.system.path as path
or
import webkitpy.common.system.path as system_path

"from webkitpy.common.system.path import abspath_to_uri" is also fine with me.

[Dirk Pranke]

Created attachment 70164 [details]
Patch

[Adam Barth]

Comment on attachment 70164 [details]
Patch

Assuming dirk meant to mark this for review.

[Dirk Pranke]

Created attachment 70166 [details]
Patch

[Dirk Pranke]

actually didn't mean to mark the earlier patch for review, I was just posting it so I could reapply it on windows and re-test; the latest patch fixes a bug in the way we were checking the output from cygpath.

this one's good to go.

[Adam Barth]

Comment on attachment 70166 [details]
Patch

okiedokes

[Dirk Pranke]

Comment on attachment 70166 [details]
Patch

Clearing flags on attachment: 70166

Committed r69363: <http://trac.webkit.org/changeset/69363>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.