Bug 52482 – rebaseline-chromium-webkit-tests: use filesystem objects

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

[Dirk Pranke]

rebaseline-chromium-webkit-tests: use filesystem objects

[Dirk Pranke]

Created attachment 79010 [details]
Patch

[Dirk Pranke]

Created attachment 79011 [details]
fix minor bug in test_generate_baseline_links

[Mihai Parparita]

Comment on attachment 79011 [details]
fix minor bug in test_generate_baseline_links

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:171


Should this handle paths that are substrings of one another?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:174
> +    def split(self, path):

This looks like a helper method, not one that's exposed in the Filesystem interface. Call it _split?

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:111
> +        filesystem.rmtree(html_directory, True)

Filesystem.rmtree doesn't seem to have a second boolean argument.

[Dirk Pranke]

Created attachment 79041 [details]
update w/ changes in response to Mihai's comments

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 79011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79011&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:171
> 
> 
> Should this handle paths that are substrings of one another?
>

I'm sorry, I don't understand this. It looks like you're commenting on rmtree()? I'm not sure how what you wrote is relevant to what that routine is doing (rm -fr path).
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:174
> > +    def split(self, path):
> 
> This looks like a helper method, not one that's exposed in the Filesystem interface. Call it _split?
>

Done.
 
> > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:111
> > +        filesystem.rmtree(html_directory, True)
> 
> Filesystem.rmtree doesn't seem to have a second boolean argument.

Good catch. Done. Thanks for the feedback!

[Tony Chang]

Comment on attachment 79041 [details]
update w/ changes in response to Mihai's comments

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:115
>                      os.rmdir(self._directory_path)
>  
> +
> +

Nit: Did you mean to add this whitespace?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:171
> +    def remove(self, path):
> +        if self.files[path] is None:
> +            self._raise_not_found(path)
> +        self.files[path] = None

It seems weird to have None entries in self.files.  Can you use 'del' instead to remove the entry?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:176
> +    def rmtree(self, path):
> +        for f in self.files:
> +            if f.startswith(path):
> +                self.files[f] = None

You probably want to use 'del' here too (along with self.files.keys() so you don't confuse the iterator).

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:203
> +    def __exit__(self, type, value, traceback):
> +        # FIXME: should we close() here?

I think that's normally what happens in __exit__.  Seems like it would be good to match the built-in file() implementation, although I guess it doesn't matter much either way.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:148
> +            self._running_port._filesystem.exists = lambda x: True

Do we need to undo this change so tests that run after this don't have a patched _filesystem.exists?

[Eric Seidel (no email)]

Comment on attachment 79041 [details]
update w/ changes in response to Mihai's comments

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

> Tools/Scripts/webkitpy/common/system/filesystem.py:176
> +    def rmtree(self, path):

I could have *sworn* I just added this function in a commit 2 days ago?  Maybe my commit didn't land?  Are you sure your'e up to date?

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 79041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79041&action=review
> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:115
> >                      os.rmdir(self._directory_path)
> >  
> > +
> > +
> 
> Nit: Did you mean to add this whitespace?
> 

No, probably not. I'll fix it.

> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:171
> > +    def remove(self, path):
> > +        if self.files[path] is None:
> > +            self._raise_not_found(path)
> > +        self.files[path] = None
> 
> It seems weird to have None entries in self.files.  Can you use 'del' instead to remove the entry?
>

This is intentional, actually. It allows me to capture the "file does not exist" concept separately from trying access a file/path I wasn't expecting (which will raise a KeyError exception and cause the test to fail.)
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:176
> > +    def rmtree(self, path):
> > +        for f in self.files:
> > +            if f.startswith(path):
> > +                self.files[f] = None
> 
> You probably want to use 'del' here too (along with self.files.keys() so you don't confuse the iterator).
>

See above.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:203
> > +    def __exit__(self, type, value, traceback):
> > +        # FIXME: should we close() here?
> 
> I think that's normally what happens in __exit__.  Seems like it would be good to match the built-in file() implementation, although I guess it doesn't matter much either way.
>

I agree with everything you just wrote ;)
 
> > Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:148
> > +            self._running_port._filesystem.exists = lambda x: True
> 
> Do we need to undo this change so tests that run after this don't have a patched _filesystem.exists?

Not really, because it gets overwritten in the next call to make_generator(). running_port is really more of a local variable than a member variable. I'll change it accordingly.

[Dirk Pranke]

Created attachment 79049 [details]
update w/ feedback from tony

[Mihai Parparita]

(In reply to comment #5)
> I'm sorry, I don't understand this. It looks like you're commenting on rmtree()? I'm not sure how what you wrote is relevant to what that routine is doing (rm -fr path).

I mean that if you have /path/foo/bar and /path/foo2/bar in the mock filesystem, and you call rmtree('/path/foo') your function would remove /path/foo2/bar too, even though it's not under that tree. files_under avoids this by appending a trailing slash to the passed in path if it doesn't have one already.

(In reply to comment #7)
> I could have *sworn* I just added this function in a commit 2 days ago?  Maybe my commit didn't land?  Are you sure your'e up to date?

Yes, remove_tree was added by http://trac.webkit.org/changeset/75760. Incidentally, it has the same problem as mentioned above, and I think I like Dirk's scheme of naming methods in Filesystem after the os[.path] functions that they wrap, so perhaps renaming it to rmtree is best.

[Dirk Pranke]

(In reply to comment #10)
> (In reply to comment #5)
> > I'm sorry, I don't understand this. It looks like you're commenting on rmtree()? I'm not sure how what you wrote is relevant to what that routine is doing (rm -fr path).
> 
> I mean that if you have /path/foo/bar and /path/foo2/bar in the mock filesystem, and you call rmtree('/path/foo') your function would remove /path/foo2/bar too, even though it's not under that tree. files_under avoids this by appending a trailing slash to the passed in path if it doesn't have one already.
> 

Ah, good point. I will fix that.

> (In reply to comment #7)
> > I could have *sworn* I just added this function in a commit 2 days ago?  Maybe my commit didn't land?  Are you sure your'e up to date?
> 
> Yes, remove_tree was added by http://trac.webkit.org/changeset/75760. Incidentally, it has the same problem as mentioned above, and I think I like Dirk's scheme of naming methods in Filesystem after the os[.path] functions that they wrap, so perhaps renaming it to rmtree is best.

My tree was last synced to 75748, so I must've just missed it. I'll sync things up, but like Mihai said, I much prefer it being called rmtree().

[Dirk Pranke]

Created attachment 79220 [details]
update w/ more feedback from Mihai, merge to tip of tree

[Dirk Pranke]

(In reply to comment #12)
> Created an attachment (id=79220) [details]
> update w/ more feedback from Mihai, merge to tip of tree

Latest patch addresses Mihai's bug/fix, updates to tip-of-tree, renames remove_tree to rmtree (and updates the reference in tools/...).

[Dirk Pranke]

Comment on attachment 79220 [details]
update w/ more feedback from Mihai, merge to tip of tree

Clearing flags on attachment: 79220

Committed r76050: <http://trac.webkit.org/changeset/76050>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.