[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 1 19:10:51 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=50098
--- Comment #13 from James Kozianski <koz at chromium.org> 2010-12-01 19:10:50 PST ---
(In reply to comment #9)
> (In reply to comment #7)
> > Thanks for the prompt and thorough review, Adam!
> >
> > (In reply to comment #6)
> > > (From update of attachment 74983 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=74983&action=review
> > >
> > > > 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.
>
> We should never have let rebaseline_chromium_webkit_tests into the tree. It's wrong at so many levels.
>
> The mapping between LayoutTest/platform directories and buildbots probably belongs somewhere in the port abstraction. You'll find that we already have two port abstractions (shame on us), but we should be able to teach one fo them to find it's buildbot.
Yep, I've moved these mappings into the webkit.py and chromium.py Port implementations (the layout_tests/port implementations.
>
> > > > 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.
>
> It's certainly fine to do that in a separate patch, but we're not going to want to make the situation worse by landing this patch before that one. That's how we accumulate these technical debts.
Okay, no worries but from the rest of the comments it looks like this tree might be a bit simplistic. Once we know what the right thing to do is I'll either make a patch to rewrite the baseline_search_path() methods to be in terms of a tree like this one, or I'll generate a tree (or graph) from the baseline_search_path()s at runtime.
>
> > > > 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.
>
> Ok, but how does that scale when we add more ports / platforms to the project? Will someone have to hunt around all of webkitpy to find all the port-specific logic and update it? It seems better to centralize that knowledge in the port abstraction.
I'm happy to have this tree live in Port, that's the right place for it. This won't lead to people needing to hunt for any port-specific stuff - all of the logic surrounding fallbacks (the structure and what platforms are real platforms versus placeholder platforms) will be defined in this tree, which will live in the Port base class.
>
> > > > 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.
>
> I meant something in buildbot.py. I'm not sure exactly what. The current abstraction might be something as simple as a string, but that doesn't mean it shouldn't be improved.
I've added a results() method to the buildbot.Build class, which returns the ResultSet of that build's '-actual' test results. The only thing I can find in buildbot.py that deals with tests is Build.layout_test_results(), which returns a LayoutTestResults object, which deals with test names, but I don't think it is better to have that return Results instead.
>
> > > > 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.
>
> Do you mean http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/tool/commands/rebaseline.py ?
>
> The only thing that file imports from layout_tests is webkitpy.layout_tests.port, but that's because webkitpy.layout_tests.port shouldn't be in layout_tests. :P
>
> Some of these invariants aren't perfect, but we'd like to make them better over time.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list