nrwt: add more unit tests for rebaseline-chromium-webkit-tests
Created attachment 79965 [details] Patch
Created attachment 79974 [details] Patch
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 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?
(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.
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.
(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.
Created attachment 79987 [details] hand-test non-mocked code paths, fix a few bugs
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.
(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 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.
Created attachment 80489 [details] use filesets
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).
Created attachment 80501 [details] update comments/docstring for real_main()
Committed r77095: <http://trac.webkit.org/changeset/77095>