Bug 53326 - webkitpy: update fileset, zipfileset to be usable by rebaseline-chromium-webkit-tests
Summary: webkitpy: update fileset, zipfileset to be usable by rebaseline-chromium-webk...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 53040
  Show dependency treegraph
 
Reported: 2011-01-28 12:56 PST by Dirk Pranke
Modified: 2011-01-30 15:33 PST (History)
6 users (show)

See Also:


Attachments
Patch (14.00 KB, patch)
2011-01-28 12:57 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
add tests for fs.sep, make it readonly (15.79 KB, patch)
2011-01-28 14:53 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ more review feedback from eseidel (15.68 KB, patch)
2011-01-28 15:30 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-01-28 12:56:09 PST
webkitpy: update fileset, zipfileset to be usable by rebaseline-chromium-webkit-tests
Comment 1 Dirk Pranke 2011-01-28 12:57:01 PST
Created attachment 80486 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-01-28 13:33:30 PST
Comment on attachment 80486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review

> Tools/Scripts/webkitpy/common/system/fileset.py:40
> +    def close(self):

Why?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
> +        # FIXME: might want tests for this and/or a better comment about how
> +        # it works.

go go punchcards. :)

> Tools/Scripts/webkitpy/common/system/ospath.py:35
> +def relpath(path, start_path, os_path_abspath=None, sep=None):

We should probably just move this onto filesystem.py now that we have it.

> Tools/Scripts/webkitpy/common/system/zipfileset.py:43
> +        return (temp_file, zipfile.ZipFile(temp_file))

Confused wehre this is used.

> Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py:67
> +        return (None, result)

Confused where thsi is used.
Comment 3 Eric Seidel (no email) 2011-01-28 13:36:23 PST
Comment on attachment 80486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review

I think this needs a bit more testing, but in general looks good!  r- cause I'd like to see at least one tst of the .sep support.

> Tools/Scripts/webkitpy/common/system/filesystem.py:206
> +        """Returns the relative path from the starting point."""

This seems unhelpful.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:56
> +        idx = path.rfind(self.sep)

Seems we should test this new .sep support somewhere.

> Tools/Scripts/webkitpy/common/system/zipfileset.py:53
> +    def close(self):

Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work).
Comment 4 Dirk Pranke 2011-01-28 13:38:29 PST
(In reply to comment #2)
> (From update of attachment 80486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review
> 
> > Tools/Scripts/webkitpy/common/system/fileset.py:40
> > +    def close(self):
> 
> Why?
> 

The use is clearer in the patch in 53040. The zipfileset downloads the URL into a temp file, but provides no way for the tempfile to be subsequently deleted, which means that the archives get leaked into the filesystem over time. In theory they may get cleaned up when the machine reboots, but that tends to be dependent on the machine configuration (/tmp and friends are not guaranteed to be cleaned up), and if feels wrong to me to rely on that.

> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
> > +        # FIXME: might want tests for this and/or a better comment about how
> > +        # it works.
> 
> go go punchcards. :)
> 
> > Tools/Scripts/webkitpy/common/system/ospath.py:35
> > +def relpath(path, start_path, os_path_abspath=None, sep=None):
> 
> We should probably just move this onto filesystem.py now that we have it.
> 

Yeah, will do in a separate patch.

> > Tools/Scripts/webkitpy/common/system/zipfileset.py:43
> > +        return (temp_file, zipfile.ZipFile(temp_file))
> 
> Confused wehre this is used.
>

As per above.
 
> > Tools/Scripts/webkitpy/common/system/zipfileset_unittest.py:67
> > +        return (None, result)
> 
> Confused where thsi is used.
Comment 5 Dirk Pranke 2011-01-28 13:40:07 PST
(In reply to comment #3)
> (From update of attachment 80486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review
> 
> I think this needs a bit more testing, but in general looks good!  r- cause I'd like to see at least one tst of the .sep support.
>

Okay, I can add some unit tests.
 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:206
> > +        """Returns the relative path from the starting point."""
> 
> This seems unhelpful.
>

Will delete.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:56
> > +        idx = path.rfind(self.sep)
> 
> Seems we should test this new .sep support somewhere.
> 

Will do.

> > Tools/Scripts/webkitpy/common/system/zipfileset.py:53
> > +    def close(self):
> 
> Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work).

It's not clear to me that the lifetime of the zipfile will fit cleanly into a with block, but it's not much work to add it. I can do that in a separate patch and I'll add a FIXME to track it for now.
Comment 6 Eric Seidel (no email) 2011-01-28 13:41:56 PST
(In reply to comment #5)
> (In reply to comment #3)
> > Now that we have close, seems we might want to have __being__ __end__ support (or whatever it takes to have "with" work).
> 
> It's not clear to me that the lifetime of the zipfile will fit cleanly into a with block, but it's not much work to add it. I can do that in a separate patch and I'll add a FIXME to track it for now.

Yeah, like many of my review comments, I was just musing to the void about what the perfect platonic zipfileset might look like.  My appologies that my musings are often interpretable as requests for action.  Certainly not needed in this patch. :)
Comment 7 Dirk Pranke 2011-01-28 14:09:17 PST
(In reply to comment #5)
> (In reply to comment #3)
> > (From update of attachment 80486 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=80486&action=review
> > 
> > I think this needs a bit more testing, but in general looks good!  r- cause I'd like to see at least one tst of the .sep support.
> >
> 
> Okay, I can add some unit tests.
>

Before I go off and write stuff .. is there something specific you're concerned about testing, or is this just a general concern for coverage? The code paths are well-covered under the layout_tests unit tests, but I have no problem adding unit tests here as well, I just wanted to make sure I was addressing what your concern really was.
Comment 8 Eric Seidel (no email) 2011-01-28 14:22:29 PST
Comment on attachment 80486 [details]
Patch

I just see you adding a .sep feature, and I don't see it used anywhere, that's all.  No specific need.  Just would like to see a test which uses a different .sep
Comment 9 Dirk Pranke 2011-01-28 14:27:40 PST
(In reply to comment #8)
> (From update of attachment 80486 [details])
> I just see you adding a .sep feature, and I don't see it used anywhere, that's all.  No specific need.  Just would like to see a test which uses a different .sep

Ah, I didn't add it, it was already there. All I did was replace places where we still had hard-coded '/' strings with self.sep. The property is meant to be read-only (I suppose I could code it as such) and public just so people can refer to it if they need to rather than using os.sep and potentially introducing bugs in unit tests on windows.

At some point it might be interesting to actually change the '/' to something else and see what happens.
Comment 10 Dirk Pranke 2011-01-28 14:53:55 PST
Created attachment 80499 [details]
add tests for fs.sep, make it readonly
Comment 11 Dirk Pranke 2011-01-28 14:56:39 PST
(In reply to comment #9)
> At some point it might be interesting to actually change the '/' to something else and see what happens.

Every once in a while I think this and then I realize that you can't do this without breaking the constructors, because the files dict relies on the keys having '/' as the separator.

I.e., we could change self.sep, but every unit test that relied on a pre-existing filename with a path separator in it would break.
Comment 12 Eric Seidel (no email) 2011-01-28 15:12:05 PST
Comment on attachment 80499 [details]
add tests for fs.sep, make it readonly

View in context: https://bugs.webkit.org/attachment.cgi?id=80499&action=review

Still looks OK.  See nits below.

> Tools/Scripts/webkitpy/common/system/filesystem.py:55
> +    def _get_sep(self):
> +        return self._sep
> +
> +    sep = property(_get_sep, doc="pathname separator")

I'm not sure making it read-only is really worth the trouble, but it's not horrible either.

> Tools/Scripts/webkitpy/common/system/filesystem.py:211
> +        """Returns the relative path from the starting point."""

Doesn't add anything, really.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:55
> +    def _get_sep(self):
> +        return self._sep
> +
> +    sep = property(_get_sep, doc="pathname separator")

Sigh, more boilerplate.  There really isn't a way to do this with less code?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:62
>      def _split(self, path):
> -        idx = path.rfind('/')
> +        idx = path.rfind(self.sep)
>          return (path[0:idx], path[idx + 1:])

Isn't this just path.rsplit(self.sep, max=1)?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:66
> +        if path.endswith(self.sep):
> +            return path[:-1]

I think you just want return path.rstrip('/'), no?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:149
> +        # FIXME: might want tests for this and/or a better comment about how
> +        # it works.

I think the patch ordering might have gotten mixed locally.

> Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:175
> +        self.assertEquals(fs.sep, os.sep)
> +        self.assertEquals(fs.join("foo", "bar"),
> +                          os.path.join("foo", "bar"))

I guess I was more intersted in seeing a FileSystem instantiated with a custom sep and seen that it was used.  FileSystem(sep="#"), fs.join('foo', 'bar') == 'foo#bar', etc.

> Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:184
> +        try:
> +            fs.sep = ' '
> +            self.assertTrue(False, msg="fs.sep isn't readonly")
> +        except AttributeError, e:
> +            pass

there is an assertRaises which seems more appropriate here (assuming you keep the read-only stuff, which I'm not sure is necessary, but is up to you.
Comment 13 Dirk Pranke 2011-01-28 15:25:01 PST
(In reply to comment #12)
> (From update of attachment 80499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80499&action=review
> 
> Still looks OK.  See nits below.
> 
> > Tools/Scripts/webkitpy/common/system/filesystem.py:55
> > +    def _get_sep(self):
> > +        return self._sep
> > +
> > +    sep = property(_get_sep, doc="pathname separator")
> 
> I'm not sure making it read-only is really worth the trouble, but it's not horrible either.
> 

I think it probably makes things a little more obvious.

> > Tools/Scripts/webkitpy/common/system/filesystem.py:211
> > +        """Returns the relative path from the starting point."""
> 
> Doesn't add anything, really.
>

Whoops, meant to delete that.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:55
> > +    def _get_sep(self):
> > +        return self._sep
> > +
> > +    sep = property(_get_sep, doc="pathname separator")
> 
> Sigh, more boilerplate.  There really isn't a way to do this with less code?
>

Unfortunately, not as far as I know :( Still think it's probably worth it.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:62
> >      def _split(self, path):
> > -        idx = path.rfind('/')
> > +        idx = path.rfind(self.sep)
> >          return (path[0:idx], path[idx + 1:])
> 
> Isn't this just path.rsplit(self.sep, max=1)?
> 

Yeah, I think that works.

> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:66
> > +        if path.endswith(self.sep):
> > +            return path[:-1]
> 
> I think you just want return path.rstrip('/'), no?
> 

rstrip will strip all trailing '/' rather than just one. That may or may not be the right thing to do in the mock system, so I'll probably just leave things the way they are.

> > Tools/Scripts/webkitpy/common/system/filesystem_mock.py:149
> > +        # FIXME: might want tests for this and/or a better comment about how
> > +        # it works.
> 
> I think the patch ordering might have gotten mixed locally.
> 

Maybe.

> > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:175
> > +        self.assertEquals(fs.sep, os.sep)
> > +        self.assertEquals(fs.join("foo", "bar"),
> > +                          os.path.join("foo", "bar"))
> 
> I guess I was more intersted in seeing a FileSystem instantiated with a custom sep and seen that it was used.  FileSystem(sep="#"), fs.join('foo', 'bar') == 'foo#bar', etc.
>

Yeah, understood. As per comment #10 and #11, that isn't the intended usage and probably would be bad. I suppose I could change the constructor to substitute self.sep for '/' in the keys.
 
> > Tools/Scripts/webkitpy/common/system/filesystem_unittest.py:184
> > +        try:
> > +            fs.sep = ' '
> > +            self.assertTrue(False, msg="fs.sep isn't readonly")
> > +        except AttributeError, e:
> > +            pass
> 

To use assertRaises I need to stuff the assignment into a local function, but I can do that.
Comment 14 Dirk Pranke 2011-01-28 15:30:40 PST
Created attachment 80509 [details]
update w/ more review feedback from eseidel
Comment 15 Eric Seidel (no email) 2011-01-28 16:31:48 PST
Comment on attachment 80509 [details]
update w/ more review feedback from eseidel

OK.
Comment 16 Dirk Pranke 2011-01-30 15:07:13 PST
Comment on attachment 80509 [details]
update w/ more review feedback from eseidel

Clearing flags on attachment: 80509

Committed r77093: <http://trac.webkit.org/changeset/77093>
Comment 17 Dirk Pranke 2011-01-30 15:07:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Dirk Pranke 2011-01-30 15:33:07 PST
Committed r77096: <http://trac.webkit.org/changeset/77096>