webkitpy: need a filesystem wrapper object for either mocking and dependency isolation
Created attachment 71582 [details] Patch
Comment on attachment 71582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=71582&action=review Generally, I like the approach. I'm sad about have a second way to find the checkout_root. There should only be one. > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:54 > + def path_from_webkit_base(self, *comps): > + """Returns the absolute path for webkit_base_dir() + comps.""" > + return os.path.join(self.webkit_base_dir(), *comps) Hum... I wonder if we should put webkit-specific knowledge in a separate layer. > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:74 > + def read_file(self, filename, encoding=None): Do we want different methods for reading binary and reading text? > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:103 > + def webkit_base_dir(self): > + """Returns the absolute path to the top of the WebKit tree. > + > + Raises an AssertionError if the top dir can't be determined. > + > + """ > + # Note that we don't use common/checkout/scm for this in order to > + # allow the layout tests to be run without being in an actual checkout. > + if not self._webkit_base_dir: > + try: > + this_dir = os.path.abspath(sys.path[0]) > + index = this_dir.index("WebKitTools") > + self._webkit_base_dir = this_dir[0:index] > + except ValueError: > + raise AssertionError("could not determine webkit_base_dir()") This seems to duplicate logic from scm.checkout_root. However, this code is wrong if you randomly have a folder named WebKitTools somewhere. It seems better to put think knowledge at a higher level that can use scm to figure out the checkout_root. > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:58 > + def test_isdir__true(self): why the double __ ?
(In reply to comment #2) > (From update of attachment 71582 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=71582&action=review > > Generally, I like the approach. I'm sad about have a second way to find the checkout_root. There should only be one. > There seems to be a need/desire to be able to run stuff without an actual checkout (meaning, you don't have .git or .svn directories) and so we can't use scm.checkout_root. I've discussed this w/ Eric and Tony before, but this might be new to you. My thinking was that scm.checkout would subclass this class and either override the webkit_base_dir method to key off of the SCM stuff, or just use whatever we do here. > > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:54 > > + def path_from_webkit_base(self, *comps): > > + """Returns the absolute path for webkit_base_dir() + comps.""" > > + return os.path.join(self.webkit_base_dir(), *comps) > > Hum... I wonder if we should put webkit-specific knowledge in a separate layer. > I don't think so. This code isn't intended to be used outside of webkit, and everything inside webkit will need this. > > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:74 > > + def read_file(self, filename, encoding=None): > > Do we want different methods for reading binary and reading text? > I thought about that; the port class does have different methods for getting image data, text data, and checksums, for example. But I think it's perhaps better to have a generic method here and tighter data type binding at the next layer up. > > WebKitTools/Scripts/webkitpy/common/system/filesystem.py:103 > > + def webkit_base_dir(self): > > + """Returns the absolute path to the top of the WebKit tree. > > + > > + Raises an AssertionError if the top dir can't be determined. > > + > > + """ > > + # Note that we don't use common/checkout/scm for this in order to > > + # allow the layout tests to be run without being in an actual checkout. > > + if not self._webkit_base_dir: > > + try: > > + this_dir = os.path.abspath(sys.path[0]) > > + index = this_dir.index("WebKitTools") > > + self._webkit_base_dir = this_dir[0:index] > > + except ValueError: > > + raise AssertionError("could not determine webkit_base_dir()") > > This seems to duplicate logic from scm.checkout_root. However, this code is wrong if you randomly have a folder named WebKitTools somewhere. It seems better to put think knowledge at a higher level that can use scm to figure out the checkout_root. > See above. I can tighten this to more of the path name (e.g., WebkitTools/Scripts/webkitpy) to make it quite unlikely to find duplicates. > > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:58 > > + def test_isdir__true(self): > > why the double __ ? Just a convention I use sometimes to separate the routine I'm testing from the type of test or what I expect the test to be doing.
Created attachment 71609 [details] Patch
(In reply to comment #4) > Created an attachment (id=71609) [details] > Patch This rev removes any knowledge of webkit'isms or base directories. It's a pure wrapper class.
Created attachment 71814 [details] Patch
Comment on attachment 71814 [details] Patch 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.
(In reply to comment #7) > (From update of attachment 71814 [details]) > 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. > Okay. It was cut&paste from a template ;) > > 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. > eh. Ignoring the error seems like a fairly non-standard thing, so I'm not too surprised by this. > > 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? > I'm inclined to roll the path module directly into this to reduce the number of filesystem and path-related modules the programmer needs to know about. I don't feel strongly about it, though, so I could leave this out. > > 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? > Will fix. See below. > 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. > Will take a look. > > 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? > The test doesn't work (hardcoded unix-style path). I will clarify the comment. > > 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. > Seems out of scope to me. I am trusting that the underlying codecs.open() implementation does the right thing. It's conceivable that one could implement read_file on top of file() and not codecs, in which case this would be broken, but that seems like more trouble than it is worth. > > 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. > To me it feels clearer and more explicit to create the filesystem as a local variable in the routine. Otherwise I have to stop and think about where the variable came from, was it left in a clean state (which it should be from the teardown), etc. > > 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. For some reason I think I was thinking that by specifying the prefix we might get conflicts when running multiple concurrent tests. I don't know why I was thinking that. Will fix.
Created attachment 72131 [details] Patch
(In reply to comment #9) > Created an attachment (id=72131) [details] > Patch Patch uploaded w/ changes made due to review feedback. I've incorporated the TemporaryDirectory class as a new FileSystem.mkdtemp() method. I added some more comments/fixme's about merging the path modules into this module as well.
Comment on attachment 72131 [details] Patch 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.
(In reply to comment #11) > (From update of attachment 72131 [details]) > 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. > The code is used by the patch to 48280. For the comment about the /bin directory, see below. > > 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. > I'm not sure if this is a comment about a few specific docstrings, or all of them. I will tighten up the ones I think can/should be tighter. Let me know if you want to see more. > > 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. Yes, it would be. I would have thought that running test-webkitpy as root would not be a good idea. I can put in a check on setuid to bail out if necessary. That said, this was the only way I could think of to be guaranteed that trying to create a directory would fail (without mocking out the underlying implementation and making the test case too white-box-ish). Can you think of some other way to test this, or would you go ahead with the os-level mocking, or would you just not test this at all?
Created attachment 72568 [details] Patch
Created attachment 73010 [details] Patch
Comment on attachment 73010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73010&action=review Thanks Dirk. This looks great. > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:99 > + # Clean up. > + os.rmdir(sub_path) Do we want to use a finally here to make sure we clean up? I'm not sure if this is covered by the with statement.
(In reply to comment #15) > > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:99 > > + # Clean up. > > + os.rmdir(sub_path) > > Do we want to use a finally here to make sure we clean up? I'm not sure if this is covered by the with statement. The with statement covers it.
(In reply to comment #16) > (In reply to comment #15) > > > WebKitTools/Scripts/webkitpy/common/system/filesystem_unittest.py:99 > > > + # Clean up. > > > + os.rmdir(sub_path) > > > > Do we want to use a finally here to make sure we clean up? I'm not sure if this is covered by the with statement. > > The with statement covers it. Whoops. I was confused by the comments into thinking the os.rmdir was in test_maybe_make_directories__failure rather than the previous routine. Yes, if the test fails and the sub_path is actually created, then we won't clean up properly. I will fix this before landing.
Committed r71383: <http://trac.webkit.org/changeset/71383>