[webkit-reviews] review granted: [Bug 52482] rebaseline-chromium-webkit-tests: use filesystem objects : [Attachment 79041] update w/ changes in response to Mihai's comments

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 52482: rebaseline-chromium-webkit-tests: use filesystem objects
https://bugs.webkit.org/show_bug.cgi?id=52482

Attachment 79041: update w/ changes in response to Mihai's comments
https://bugs.webkit.org/attachment.cgi?id=79041&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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.p
y: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?