[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