[webkit-reviews] review denied: [Bug 48144] webkitpy: need a filesystem wrapper object for either mocking and dependency isolation : [Attachment 71814] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 48144: webkitpy: need a filesystem wrapper object for either mocking and
dependency isolation
https://bugs.webkit.org/show_bug.cgi?id=48144

Attachment 71814: Patch
https://bugs.webkit.org/attachment.cgi?id=71814&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71814&action=review

r- for the nits and the seeming touching of the source dir.

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:1
> +#!/usr/bin/env python

Not needed.

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:68
> +    def maybe_make_directory(self, *path):
> +	   """Creates the specified directory if it doesn't already exist."""

It's pretty lame that python doesn't have this already.

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:77
> +    def path_to_uri(self, path):
> +	   "Converts path to an absolute path and turns it into a filename
URI."
> +	   return path_module.abspath_to_uri(os.path.abspath(path))

I'm confused why this is here?

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:68
> +	   # FIXME: Theoretically this might not be writable.
> +	   base_path = tempfile.mktemp(prefix='filesystem_unittest_',
> +				       dir=self._this_dir)

I'm confused. Does this use the current directory?  Shouldn't it use the
standard temp directory location?

I recently added a TemporaryDirectory() class which might be useful here.  It
needs to move to a shared location, but it allows you to use temporary
directories with with_statement so they self-delete.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:91
> +	   # FIXME: Make this work under cygwin, win32.
> +	   if sys.platform in ('darwin', 'linux2'):
> +	       self.assertRaises(OSError, fs.maybe_make_directory,
> +				 "/bin/foo")

Is it that the test doesn't work on cygwin/win32? or does the
maybe_make_direcotry code not?

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:100
> +	   contents = fs.read_file(self._this_file, encoding="utf-8")
> +	   self.assertTrue('SOME_REALLY_UNLIKELY_STRING' in contents)
> +

Seems we should test unicode here?  Or is that out of scope?  We'd have to
write a unicode file and then read it back.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:103
> +	   fs = FileSystem()
> +	   self.assertRaises(IOError, fs.read_file, self._missing_file)

Probably makes sense to have a setUp() call which sets self.file_system to
FileSystem() instead of creating a new fs manually in every test.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:112
> +	       # FIXME: Theoretically this might not be writable.
> +	       path = tempfile.mktemp(prefix='tree_unittest_',
> +				      dir=self._this_dir)
> +	       fs.write_file(path, 'hello')

I'm confused again.  We should never be touching the source directory during
tests.