Bug 52482 - rebaseline-chromium-webkit-tests: use filesystem objects
Summary: rebaseline-chromium-webkit-tests: use filesystem objects
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: 52487
  Show dependency treegraph
 
Reported: 2011-01-14 14:14 PST by Dirk Pranke
Modified: 2011-01-18 13:10 PST (History)
5 users (show)

See Also:


Attachments
Patch (33.33 KB, patch)
2011-01-14 15:06 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
fix minor bug in test_generate_baseline_links (33.29 KB, patch)
2011-01-14 15:09 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ changes in response to Mihai's comments (33.35 KB, patch)
2011-01-14 17:51 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ feedback from tony (33.22 KB, patch)
2011-01-14 20:01 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ more feedback from Mihai, merge to tip of tree (34.24 KB, patch)
2011-01-17 14:59 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-14 14:14:03 PST
rebaseline-chromium-webkit-tests: use filesystem objects
Comment 1 Dirk Pranke 2011-01-14 15:06:33 PST
Created attachment 79010 [details]
Patch
Comment 2 Dirk Pranke 2011-01-14 15:09:21 PST
Created attachment 79011 [details]
fix minor bug in test_generate_baseline_links
Comment 3 Mihai Parparita 2011-01-14 16:45:44 PST
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.
Comment 4 Dirk Pranke 2011-01-14 17:51:53 PST
Created attachment 79041 [details]
update w/ changes in response to Mihai's comments
Comment 5 Dirk Pranke 2011-01-14 17:56:29 PST
(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!
Comment 6 Tony Chang 2011-01-14 18:59:50 PST
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?
Comment 7 Eric Seidel (no email) 2011-01-14 19:22:40 PST
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?
Comment 8 Dirk Pranke 2011-01-14 19:54:43 PST
(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.
Comment 9 Dirk Pranke 2011-01-14 20:01:53 PST
Created attachment 79049 [details]
update w/ feedback from tony
Comment 10 Mihai Parparita 2011-01-14 23:40:52 PST
(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.
Comment 11 Dirk Pranke 2011-01-17 14:12:36 PST
(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().
Comment 12 Dirk Pranke 2011-01-17 14:59:14 PST
Created attachment 79220 [details]
update w/ more feedback from Mihai, merge to tip of tree
Comment 13 Dirk Pranke 2011-01-17 15:00:02 PST
(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/...).
Comment 14 Dirk Pranke 2011-01-18 13:10:41 PST
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>
Comment 15 Dirk Pranke 2011-01-18 13:10:47 PST
All reviewed patches have been landed.  Closing bug.