Bug 53040 - nrwt: add more unit tests for rebaseline-chromium-webkit-tests
Summary: nrwt: add more unit tests for rebaseline-chromium-webkit-tests
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: 53036 53326
Blocks: 51222
  Show dependency treegraph
 
Reported: 2011-01-24 13:21 PST by Dirk Pranke
Modified: 2011-01-30 15:16 PST (History)
6 users (show)

See Also:


Attachments
Patch (30.85 KB, patch)
2011-01-24 13:24 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (30.58 KB, patch)
2011-01-24 14:06 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
hand-test non-mocked code paths, fix a few bugs (31.49 KB, patch)
2011-01-24 16:34 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
use filesets (31.02 KB, patch)
2011-01-28 13:20 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
update comments/docstring for real_main() (34.91 KB, patch)
2011-01-28 15:05 PST, Dirk Pranke
mihai: review+
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-24 13:21:59 PST
nrwt: add more unit tests for rebaseline-chromium-webkit-tests
Comment 1 Dirk Pranke 2011-01-24 13:24:57 PST
Created attachment 79965 [details]
Patch
Comment 2 Dirk Pranke 2011-01-24 14:06:44 PST
Created attachment 79974 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-01-24 15:21:51 PST
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.
Comment 4 Eric Seidel (no email) 2011-01-24 15:23:55 PST
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?
Comment 5 Dirk Pranke 2011-01-24 15:38:18 PST
(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.
Comment 6 Dirk Pranke 2011-01-24 15:41:29 PST
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.
Comment 7 Dirk Pranke 2011-01-24 16:06:28 PST
(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.
Comment 8 Dirk Pranke 2011-01-24 16:34:56 PST
Created attachment 79987 [details]
hand-test non-mocked code paths, fix a few bugs
Comment 9 Dirk Pranke 2011-01-24 17:08:28 PST
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.
Comment 10 Victor Wang 2011-01-25 10:14:13 PST
(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.
Comment 11 Mihai Parparita 2011-01-26 13:22:15 PST
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.
Comment 12 Dirk Pranke 2011-01-28 13:20:05 PST
Created attachment 80489 [details]
use filesets
Comment 13 Mihai Parparita 2011-01-28 14:37:18 PST
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).
Comment 14 Dirk Pranke 2011-01-28 15:05:20 PST
Created attachment 80501 [details]
update comments/docstring for real_main()
Comment 15 Dirk Pranke 2011-01-30 15:16:55 PST
Committed r77095: <http://trac.webkit.org/changeset/77095>