Bug 52604 – new-run-webkit-tests: use filesystem wrapper in more places

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

[Dirk Pranke]

new-run-webkit-tests: use filesystem wrapper in more places

[Dirk Pranke]

Created attachment 79231 [details]
Patch

[Tony Chang]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 79313 [details]
update w/ review feedback from tony

[Dirk Pranke]

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

[Dirk Pranke]

Created attachment 79364 [details]
Patch

[Dirk Pranke]

Created attachment 79368 [details]
fix missed reference to os.path

[Dirk Pranke]

Okay, I split the walk() usage out into a separate patch (bug 52691). Can you r+ the rest of this?

[Dirk Pranke]

Created attachment 79461 [details]
fix missing parameter in MockFileSystem.basename()

[Dirk Pranke]

Committed r76150: <http://trac.webkit.org/changeset/76150>