Bug 53040 – nrwt: add more unit tests for rebaseline-chromium-webkit-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: add more unit tests for rebaseline-chromium-webkit-tests

[Dirk Pranke]

Created attachment 79965 [details]
Patch

[Dirk Pranke]

Created attachment 79974 [details]
Patch

[Eric Seidel (no email)]

This seems to overlap with work the Koz and I have recently been doing re: dealing with zip files and pulling down results from servers.

[Eric Seidel (no email)]

Comment on attachment 79974 [details]
Patch

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

> Tools/Scripts/webkitpy/common/zipper.py:34
> +class UnZipper(object):

Slightly odd that this is called Unzipper and the file is zipper.  Did you plan to add a Zipper class?  How does that relate with filesystem.create_zip?

I'm not sure why having a wrapper around zipfile is better than just passing mock zipfiles?

> Tools/Scripts/webkitpy/common/zipper_mock.py:30
> +def make_unzipper(ziphashes):

I don't understand the point of a wrapper function here.  why not just expose the class and use the constructor?

[Dirk Pranke]

(In reply to comment #3)
> This seems to overlap with work the Koz and I have recently been doing re: dealing with zip files and pulling down results from servers.

Thanks for adding him, I was about to do so. Yes, there's definitely overlap, and it's sort of a race to see who lands first. I was posting a version of this as-is and then was going to do another pass to see how close I could get it to Koz's version.

(In reply to comment #4)
> (From update of attachment 79974 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79974&action=review
> 
> > Tools/Scripts/webkitpy/common/zipper.py:34
> > +class UnZipper(object):
> 
> Slightly odd that this is called Unzipper and the file is zipper.  Did you plan to add a Zipper class?

I don't, but I figured someone would eventually.

>  How does that relate with filesystem.create_zip?

I am not aware of any such function. I hope there doesn't come to be one.

> 
> I'm not sure why having a wrapper around zipfile is better than just passing mock zipfiles?
>

Having a wrapper around the zipfile allows us to enforce a restricted API, which makes it easier to keep the zipfile interface in sync with the mock interface.

> > Tools/Scripts/webkitpy/common/zipper_mock.py:30
> > +def make_unzipper(ziphashes):
> 
> I don't understand the point of a wrapper function here.  why not just expose the class and use the constructor?

The constructor to Zipfile takes a path as an argument (the path to the zipfile). The code in rebaseline-chromium-webkit-tests creates zipfiles on the fly from paths in the filesystem, which means that it needs to be able to create objects. 

Using the maker function, I can ensure that only the objects I expect to be created are being created, and I can control what their contents would be, in a reusable manner, by having the object returned from make_unzipper have a reference to the lexically scoped dictionary in the 'ziphashes' argument.

If there's a better way to accomplish this, I'm certainly open to suggestions.

[Dirk Pranke]

Oh, also, it's worth noting that the approach I'm using (mocking out the whole filesystem, the network, the zipfile constructor, etc.) allows me to pretty easily write high-level functional tests of the entire module. This gets me good coverage without requiring me to be too aware of the implementation of the module or having to write very fragile unit tests.

[Dirk Pranke]

(In reply to comment #5)
> (In reply to comment #3)
> > This seems to overlap with work the Koz and I have recently been doing re: dealing with zip files and pulling down results from servers.
> 
> Thanks for adding him, I was about to do so. Yes, there's definitely overlap, and it's sort of a race to see who lands first. I was posting a version of this as-is and then was going to do another pass to see how close I could get it to Koz's version.
> 

Okay, Koz's code mostly creates zip files (looking at common/system/mock_zip.py in bug 50098), so there's not a huge amount of overlap; we should be able to cut&paste-merge the two files together. Note that I would probably put this file in common/, not common/system .

There's also the common/system/fileset.py module, which appears to do something vaguely zip like, but frankly I don't really understand what that file is for (and there's almost no comments in it), so hopefully koz can say better how that fits into things and if it makes some of the work in this bug redundant.

[Dirk Pranke]

Created attachment 79987 [details]
hand-test non-mocked code paths, fix a few bugs

[Dirk Pranke]

Victor, FYI ... I've added a bunch more unit tests to rebaseline-chromium-webkit-tests and needed to restructure some things. Let me know if you have any comments.

[Victor Wang]

(In reply to comment #9)
> Victor, FYI ... I've added a bunch more unit tests to rebaseline-chromium-webkit-tests and needed to restructure some things. Let me know if you have any comments.
Thanks for taking care of this.

[Mihai Parparita]

Comment on attachment 79987 [details]
hand-test non-mocked code paths, fix a few bugs

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

> Tools/Scripts/webkitpy/common/fetcher.py:34
> +class UrlFetcher(object):

Can the file be called urlfetcher.py, to mirror the class name? fetcher.py is a bit too generic.

> Tools/Scripts/webkitpy/common/zipper.py:34
> +class UnZipper(object):

I see that comparisons have already been made to the fileset classes that Koz added (they're meant for the rebaselining new tool, thus they should satisfy the needs of the old one too).

This class seems pretty similar to ZipFileSet (e.g. they both have namelist and read). I'm not seeing any barriers to just using it directly.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:894
> +    # We need to create three different Port objects over the life of this

I think this comment should stay in main(), since that's where we actually create the port objects.

[Dirk Pranke]

Created attachment 80489 [details]
use filesets

[Mihai Parparita]

Comment on attachment 80489 [details]
use filesets

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

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:888
> +    # We need to create three different Port objects over the life of this

This comment should go in main, where we actually create  the objects, or the comment should be re-done to describe the parameters of real_main.

Also, port_obj no longer exists.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:892
> +    # Then we create a rebaselining port to actual find and manage the

Typo, "actual" instead of "actually" (you can also omit the word entirely).

[Dirk Pranke]

Created attachment 80501 [details]
update comments/docstring for real_main()

[Dirk Pranke]

Committed r77095: <http://trac.webkit.org/changeset/77095>