[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.

Wed Dec 1 03:18:53 PST 2010

https://bugs.webkit.org/show_bug.cgi?id=50098

--- Comment #7 from James Kozianski <koz at chromium.org>  2010-12-01 03:18:53 PST ---
Thanks for the prompt and thorough review, Adam!

(In reply to comment #6)
> (From update of attachment 74983 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74983&action=review
> 
> This looks cool, but you're re-inventing a bunch of wheels instead of making the existing wheels more awesome.  Also, you've siloed a bunch of your code in a separate package instead of putting it in the right place in the package hierarchy where it can be used by other parts of webkitpy.
> 
> In general, the effectiveness of webkitpy should grow like n^2.  The way you've written this patch, the effectiveness is only growing like n because you're not making use of the network effects of sharing code.

Yep, I'm definitely happy to do that. Writing this script for me was a learning process about the test infrastructure as well as the webkitpy module. Having everything be separate allowed me to iterate quickly on my ideas and be confident that I'm not causing regressions in other people's code. Much of what's in webkitpy is stuff that I don't know that I don't know about, so your comments are helpful for me to know what should be merged and where.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/all_tests.py:7
> > +unittest.main()
> 
> We have some nifty unittest auto-discovery code.  Maybe we should use that here?  It's a pay to register new unit tests with the harness.

You're right - these tests will be auto-discovered by that script. I've deleted this file.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/bucket.py:209
> > +    def b(self, bucket_name, *args):
> 
> This is kind of a short function name.  Can we use something more descriptive?

This was an alias for 'make_bucket' intended to allow the call-sites to be concise so that the hierarchical structure of the calls would be easier to parse. I've removed this method and introduced a short alias at the call sites.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:34
> > +class RemoteZip(object):
> 
> This class seems general and should be in webkitpy.common.net

Done.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:42
> > +    def _load(self):
> > +        if self._zip_file is None:
> > +            self._zip_file = zipfile.ZipFile(urllib.urlretrieve(self._zip_url)[0])
> 
> Consider using NetworkTransaction to be robust to network connectivity issues.

Done.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:63
> > +class ZipFileHandle:
> 
> same here.

Done.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:100
> > +class DirectoryAsZip(object):
> 
> Also a general class that should be in webkit.common somewhere.

I've moved it to webkit.common.diraszip.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:121
> > +        f = open(os.path.join(self._path, filename))
> > +        contents = f.read()
> > +        f.close
> 
> We generally use "with" to avoid leaking file handles.  Also, we have a FileSystem class we use to abstract these operations.  Notice that you haven't specified a codec, which is something that FileSystem enforces.

Done.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:135
> > +    def delete(self, filename):
> > +        filename = os.path.join(self._path, filename)
> > +        os.remove(filename)
> 
> This seems like a security vulnerability waiting to happen...  Maybe some sort of requirement that the final filename is actually a subdirectory of self._path?

I've added a check to ensure the final filename doesn't include '..'. Is that enough?

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:139
> > +class Builder(object):
> > +    """Retrieves results from zip files stored on a buildbot builder"""
> 
> We already have an abstraction for a BuildBot.  Presumably we should add this functionality to that abstraction instead of creating a new abstraction.

I've renamed this to ResultSet and moved it to webkitpy.common.net as it is quite a lot more general than the existing BuildBot / builder abstractions. A ResultSet can be used independently of BuildBots / builders as well (for instance, the existing results in one's LayoutTests directory can be the source of a ResultSet), so the name was ill-suited in the first place.

I've also added a method to the Builder class (results()) that returns a ResultSet of the results stored on that Builder.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:168
> > +    def results_for(self, name, target_type=None):
> 
> We already have code that does this for the non-zip case.  We should merge this functionality into that code instead of having two silos.

Are you referring to webkitpy/common/net/layouttestresults.py? I don't think the code there and this code can really be merged - that code is for parsing a particular results.html file to extract the names of failing tests, whereas this code consumes zip files and provides search functionality for retrieving Result objects, which can be compared and saved to disk. Also, as mentioned above, the ResultSet code is quite general and it is useful outside of the context of BuildBot, so I would prefer to keep this in its own class.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:209
> > +class AggregateBuilder(object):
> 
> This belongs in the buildbot package somewhere.

I've moved this to webkitpy.common.net, where buildbot.py lives.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:234
> > +        return 'http://build.chromium.org/f/chromium/layout_test_results/' + builder_name + '/layout-test-results.zip'
> 
> General webkitpy code shouldn't refer to build.chromium.org.  You probably need to add this to the port abstraction.

Done. I've added a method Port.buildbot_resultset() which abstracts how the ResultSet is produced for each platform.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/builder.py:264
> > +class WebKitBuilder(object):
> > +    BUILDERS = {
> > +        'mac-snowleopard': 'SnowLeopard%20Intel%20Release%20(Tests)',
> > +        'mac-leopard': 'Leopard%20Intel%20Debug%20(Tests)',
> > +        'win': 'Windows%207%20Release%20(WebKit2%20Tests)',
> > +        'chromium-linux': 'GTK%20Linux%2032-bit%20Release',
> > +    }
> 
> webkitpy already knows about these things.  Please look at how the rest of the system knows about this stuff.

What I'm trying to express here is a mapping between platforms (as in the subdirectories of LayoutTests/platform) and the name of the builder that provides the 'canonical' test results for that platform. When a user wants to get new baselines for mac-snowleopard, what builder should those baselines be retrieved from? Looking through the code I can't find anywhere where such a mapping is defined - rebaseline_chromium_webkit_tests.py defines its own mapping for this kind of work, the current webkit-patch rebaseline script prompts the user to select a builder name and the port object itself provides a fallback order, but no mapping between platforms and builders.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:61
> > +    def _make_bucket_tree(self):
> > +        b = bucket.BucketTree()
> > +        b.b(None,
> > +            b.b('mac',
> > +                b.b('mac-snowleopard',
> > +                    b.b('mac-leopard',
> > +                        b.b('mac-tiger'))),
> > +                b.b('win',
> > +                    b.b('chromium',
> > +                        b.b('chromium-mac'),
> > +                        b.b('chromium-win',
> > +                            b.b('chromium-win-vista',
> > +                                b.b('chromium-win-xp')),
> > +                            b.b('chromium-linux',
> > +                                b.b('chromium-gpu',
> > +                                    b.b('chromium-gpu-linux'),
> > +                                    b.b('chromium-gpu-win'),
> > +                                    b.b('chromium-gpu-mac'))))))),
> > +            b.b('qt',
> > +                b.b('qt-linux'),
> > +                b.b('qt-mac'),
> > +                b.b('qt-win')),
> > +            b.b('gtk'))
> 
> Is this some kind of description of the structure of the expectations fallback tree?  If so, it should be in code that's shareable with the rest of webkitpy instead of siloed away here.

Yes, this is the fallback tree that is implied by the various implementations of Port.baseline_search_path() - it is needed to do correct deduping. I would like to see all the different implementations of that method go away and have the tree above be the authoritive fallback tree, but that seemed like it should happen in its own patch.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:63
> > +        AGGREGATE_BUCKETS = ['chromium', 'chromium-win', 'win', 'mac', 'qt', 'chromium-gpu']
> 
> Why is this magic list here?  We shouldn't have magic constants like this in the code.

These are the names of platforms that aren't targeted by any builders because they don't denote real platforms, merely convenient fallback targets. This is significant in deduping: if we have the same expectations in mac-leopard, mac-snowleopard and mac-tiger, but a different result in mac, then we can clobber the result in mac and delete the others. We couldn't do that if mac was a platform in itself, because then it having different expectations would be valid and not a duplication.

I've added a clarifying comment to explain the meaning of the list.

> 
> > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/result.py:35
> > +class Result(object):
> > +    """Represents the result of a single test on a single platform"""
> 
> We already have an abstraction for Result.

Are you referring to the webkitpy/layout_tests/test_types module? Those classes seem like they have a different usage to the ones I'm introducing here - they have no state and perform functions on filenames and file contents passed in to them. There is no point in having more than one instance of those classes, whereas the Result classes defined here are stateful and are meant to represent Results (local or remote) for the purposes of comparing them.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:29
> > +from webkitpy.layout_tests.port import factory
> 
> webkitpy.tool cannot import from webkitpy.layout_tests.  Code that's shared between these two packages should go in webkitpy.common.

Ah, okay - I was taking inspiration from the original rebaseline.py. I've moved the rebaseline package out of layout_tests and into webkitpy/tool/commands/rebaseline2.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:53
> > +            make_option("--chromium", action="store_true", dest="use_chromium_bots", help="Fetch results from the Chromium bots."),
> 
> Boo.  We should use --port to specify the port instead of making a magic special case for Chromium.

Sorry, I was imitating build-webkit's --chromium flag here. I've added Port.buildbot_resultset() so the buildbots used are controlled via the port.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:54
> > +            make_option("--webkit", action="store_false", dest="use_chromium_bots", help="Fetch results from the WebKit bots (default)."),
> 
> It doesn't make sense to have a --webkit command line argument for webkit-patch.  Everything webkit-patch does is related to WebKit!

Haha, of course :-) Removed.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:57
> > +            make_option("--list-builders", action="store_true", dest="list_builders", help="List the builders for each platform."),
> > +            make_option("--list-archives", action="store_true", dest="list_archives", help="Show the URLs of the test result archives."),
> > +            make_option("--show-archives", action="store_true", dest="show_archives", help="Show the results stored in the specified archives."),
> 
> These seem like separate commands.  Why are they arguments to this command?

I found these useful for selecting what platforms to rebase for and for seeing what results are available for those platforms, so they seemed like reasonable arguments with a similar usage to dry-run: you use them to see what the rebase command you construct will do.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:63
> > +        builder = ChromiumBuilder() if options.use_chromium_bots else WebKitBuilder()
> 
> This line of code shouldn't be here.  We should get this information by instantiating a Port object.  How will this line of code scale when there are N contributors to WebKit with as much infrastructure as Chromium?

Done.

> 
> > WebKitTools/Scripts/webkitpy/tool/commands/rebaseline2.py:73
> > +            exit()
> 
> Whenever you call exit, you might consider whether what you really want is a separate command.  Calling exit also makes this code difficult to test.

I've removed those flags.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.