Bug 52604 - new-run-webkit-tests: use filesystem wrapper in more places
Summary: new-run-webkit-tests: use filesystem wrapper in more places
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 52605
  Show dependency treegraph
 
Reported: 2011-01-17 17:42 PST by Dirk Pranke
Modified: 2011-01-19 12:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (20.18 KB, patch)
2011-01-17 17:44 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback from tony (22.73 KB, patch)
2011-01-18 12:55 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (14.24 KB, patch)
2011-01-18 17:23 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix missed reference to os.path (14.34 KB, patch)
2011-01-18 17:39 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix missing parameter in MockFileSystem.basename() (14.60 KB, patch)
2011-01-19 12:17 PST, Dirk Pranke
no flags 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-01-17 17:42:39 PST
new-run-webkit-tests: use filesystem wrapper in more places
Comment 1 Dirk Pranke 2011-01-17 17:44:01 PST
Created attachment 79231 [details]
Patch
Comment 2 Tony Chang 2011-01-18 11:02:59 PST
Comment on attachment 79231 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:51
> +        if idx == -1:
> +            idx = 0
> +        return path[idx:]

If we find '/', won't this include '/' in the returned value?  E.g., basename of '/foo/bar' would be '/bar' but it should be 'bar'.

I think you can just remove the -1 check and use path[idx + 1:].

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:78
> +    def glob(self, path):
> +        if path[-1] == '*':
> +            return [f for f in self.files if f.startswith(path[:-1])]
> +        else:
> +            return [f for f in self.files if f == path]

This doesn't handle cases where * is in the middle of the string, but it's probably good enough for now.  Maybe mention that in a comment?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:89
> +        files = self.files.keys()[:]
> +        return any(f.startswith(path) for f in files)

Why is this copy needed?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
> +    def splitext(self, path):
> +        idx = path.rfind('.')
> +        if idx == -1:
> +            idx = 0
> +        return (path[0:idx], path[idx:])

This looks like it produces the wrong output if idx == -1.  E.g., splitext("/foo/bar") should return ("/foo/bar", ""), but this returns ("", "/foo/bar").

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:148
> +        # Ripped from Python 2.6's os.walk.
> +        # equivalent to walk(top, topdown=True, followlinks=False).

If we're going to create a derivative work (i.e., copy/paste + modification), we need to include the python license and copyright.  You could do it inline in the file or add the license in a separate file-- I'm not sure how webkit normally does this.
Comment 3 Dirk Pranke 2011-01-18 12:25:01 PST
Comment on attachment 79231 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:51
>> +        return path[idx:]
> 
> If we find '/', won't this include '/' in the returned value?  E.g., basename of '/foo/bar' would be '/bar' but it should be 'bar'.
> 
> I think you can just remove the -1 check and use path[idx + 1:].

Hmm. You're right, and yet the code seems to be working as is. I'll figure out what's going on and update the patch.

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:78
>> +            return [f for f in self.files if f == path]
> 
> This doesn't handle cases where * is in the middle of the string, but it's probably good enough for now.  Maybe mention that in a comment?

Will do.

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:89
>> +        return any(f.startswith(path) for f in files)
> 
> Why is this copy needed?

That is a great question. I was testing this yesterday and getting some weird mutation errors in the unit tests as if different threads were accessing this simultaneously (as if the any() was doing a yield, which as far as I know, it doesn't, or a thread switch was happening during the walk, which as far as I know, can't happen because of the GIL). Adding the copy fixed things. I'll remove the copy and see if I can figure out what was going on.

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
>> +        return (path[0:idx], path[idx:])
> 
> This looks like it produces the wrong output if idx == -1.  E.g., splitext("/foo/bar") should return ("/foo/bar", ""), but this returns ("", "/foo/bar").

Good catch. Will fix.

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:148
>> +        # equivalent to walk(top, topdown=True, followlinks=False).
> 
> If we're going to create a derivative work (i.e., copy/paste + modification), we need to include the python license and copyright.  You could do it inline in the file or add the license in a separate file-- I'm not sure how webkit normally does this.

Good point. I'll split this into a separate file and add some sort of disclaimer and the license text.
Comment 4 Dirk Pranke 2011-01-18 12:55:13 PST
Created attachment 79313 [details]
update w/ review feedback from tony
Comment 5 Dirk Pranke 2011-01-18 12:57:03 PST
(In reply to comment #3)
> (From update of attachment 79231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79231&action=review
> 
> >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:51
> >> +        return path[idx:]
> > 
> > If we find '/', won't this include '/' in the returned value?  E.g., basename of '/foo/bar' would be '/bar' but it should be 'bar'.
> > 
> > I think you can just remove the -1 check and use path[idx + 1:].
> 
> Hmm. You're right, and yet the code seems to be working as is. I'll figure out what's going on and update the patch.
> 

The code where this was being used wasn't firing. At some point, writing unit tests for this module might be a good idea.

> >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:78
> >> +            return [f for f in self.files if f == path]
> > 
> > This doesn't handle cases where * is in the middle of the string, but it's probably good enough for now.  Maybe mention that in a comment?
> 
> Will do.
>

Done.
 
> >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:89
> >> +        return any(f.startswith(path) for f in files)
> > 
> > Why is this copy needed?
> 
> That is a great question. I was testing this yesterday and getting some weird mutation errors in the unit tests as if different threads were accessing this simultaneously (as if the any() was doing a yield, which as far as I know, it doesn't, or a thread switch was happening during the walk, which as far as I know, can't happen because of the GIL). Adding the copy fixed things. I'll remove the copy and see if I can figure out what was going on.

Of course, I can't reproduce this now. I'm not sure if that makes me feel better or worse. I've removed the copy, since it doesn't seem like it should be needed.

> 
> >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
> >> +        return (path[0:idx], path[idx:])
> > 
> > This looks like it produces the wrong output if idx == -1.  E.g., splitext("/foo/bar") should return ("/foo/bar", ""), but this returns ("", "/foo/bar").
> 
> Good catch. Will fix.
>

Fixed.
 
> >> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:148
> >> +        # equivalent to walk(top, topdown=True, followlinks=False).
> > 
> > If we're going to create a derivative work (i.e., copy/paste + modification), we need to include the python license and copyright.  You could do it inline in the file or add the license in a separate file-- I'm not sure how webkit normally does this.
> 
> Good point. I'll split this into a separate file and add some sort of disclaimer and the license text.

I've included the relevant license. Hopefully that's good enough. Do you think we need to do anything more?
Comment 6 Dirk Pranke 2011-01-18 17:23:23 PST
Created attachment 79364 [details]
Patch
Comment 7 Dirk Pranke 2011-01-18 17:39:44 PST
Created attachment 79368 [details]
fix missed reference to os.path
Comment 8 Dirk Pranke 2011-01-18 17:43:10 PST
Okay, I split the walk() usage out into a separate patch (bug 52691). Can you r+ the rest of this?
Comment 9 Dirk Pranke 2011-01-19 12:17:16 PST
Created attachment 79461 [details]
fix missing parameter in MockFileSystem.basename()
Comment 10 Dirk Pranke 2011-01-19 12:28:55 PST
Committed r76150: <http://trac.webkit.org/changeset/76150>