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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Nov 1 13:19:41 PDT 2010


Adam Barth <abarth 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 72131: Patch
https://bugs.webkit.org/attachment.cgi?id=72131&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72131&action=review

The general direction of this patch looks good.  However, I'm concerned about
the the test that writes to the /bin directory.  Also, I'd like to see this
class used in at least one place.

> WebKitTools/Scripts/webkitpy/common/system/filesystem.py:51
> +	   """Returns whether the path exists or not.
> +
> +	   Args:
> +	       path: can be absolute or relative.
> +
> +	   This is just a wrapper around os.path.exists().
> +
> +	   """

These docstrings takes up a ton of space and add very little value.

> WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:105
> +    def test_maybe_make_directory__failure(self):
> +	   fs = FileSystem()
> +
> +	   # FIXME: I'm not sure how to make this test work under cygwin or
> +	   # win32 since the whole filesystem is usually writeable.
> +	   if sys.platform in ('darwin', 'linux2'):
> +	       self.assertRaises(OSError, fs.maybe_make_directory,
> +				 "/bin/foo")

This tests seems harmful when running test-webkitpy as root.


More information about the webkit-reviews mailing list