Bug 48144 – webkitpy: need a filesystem wrapper object for either mocking and dependency isolation

• Component: New Bugs
• Status: RESOLVED FIXED

[Dirk Pranke]

webkitpy: need a filesystem wrapper object for either mocking and dependency isolation

[Dirk Pranke]

Created attachment 71582 [details]
Patch

[Adam Barth]

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 __ ?

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 71609 [details]
Patch

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 71814 [details]
Patch

[Eric Seidel (no email)]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Created attachment 72131 [details]
Patch

[Dirk Pranke]

(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.

[Adam Barth]

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.

[Dirk Pranke]

(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?

[Dirk Pranke]

Created attachment 72568 [details]
Patch

[Dirk Pranke]

Created attachment 73010 [details]
Patch

[Adam Barth]

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.

[Dirk Pranke]

(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.

[Dirk Pranke]

(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.

[Dirk Pranke]

Committed r71383: <http://trac.webkit.org/changeset/71383>