[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Dec 1 21:50:05 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=50098
--- Comment #16 from James Kozianski <koz at chromium.org> 2010-12-01 21:50:04 PST ---
(In reply to comment #12)
> (In reply to comment #9)
> >
> > 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.
> >
>
> In the absence of an obviously better place to put this, I'm inclined to agree that this belongs as a method on layout_tests/port/*, since those files are likely the basis for whatever persists into the future. That said, my comment above stands, in that this is code that has nothing to do w/ actually running the tests. I think we need to figure out how to break the Port objects up into composites.
>
> > > > > 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.
> >
>
> As Ojan has already pointed out, this logic is wrong (e.g., chromium-mac doesn't fall back to win).
>
> Unfortunately, I don't think you can even draw the logic for all of the ports as a single tree, so this approach might be fatally flawed. At least it's a DAG :)
>
> I agree with Ojan that attempting to figure out the logic in the current codebase is complicated. And, I agree that we don't want to lose port.baseline_search_path() as at least an entry point.
>
> Maybe this is something best solved with some form of composition, or mixins, or multiple inheritance. Not sure.
If I change my data structure to a DAG then we can generate it from the different baseline_search_path() implementations at runtime, and then as a next step we could replace all the implementations of baseline_search_path() with something like this...
class Port(object):
....
def baseline_search_path(self):
return DAG_OF_ALL_FALLBACK_PATHS.path_to_root_from(self.name())
>
> I also agree with Adam that it would be really nice if we didn't have two sources of truth in the tree at the same time. That said, I would be okay with it as long as you agreed to immediately fix things after this feature landed. It would be better if we could refactor/change the port code first, in a separate patch, though.
Yep, I'll try to get the DAG happening, then we won't need the two sources of truth.
>
> > > > > 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 fear you aren't aware of how we name the platform dirs. If a platform directory indicates an operating system, but not a version of an operating system, then it is meant to indicate "the most recent version of that operating system" (and it includes any older versions if the older version is either untested, or uncared about). "chromium-win" is what we use on Windows 7 bots (there is no chromium-win-7 directory). "chromium-mac" is similarly equivalent to "chromium-mac-snowleopard" (and/or Lion, which we don't really support). Same thing is true for 'mac', and 'win'. 'qt' is just a regular platform, period.
>
> So, what you said about clobbering 'mac' in the paragraph above is wrong. You would want to clobber the 'mac-leopard' and 'mac-tiger' versions of the results, but leave 'mac' and 'mac-snowleopard' (and we would assume that 'mac' now refers to Lion).
>
> The only truly "dummy" platforms are 'chromium' and 'chromium-gpu'.
I see, so the mac fallback order [mac-tiger, mac-leopard, mac-snowleopard, mac] is meant to be searching from the oldest revisions to the newest revisions of mac. Thanks for the explanation.
So this would imply that nothing in the mac directory will ever get overwritten by this rebaseline script because we don't have mac-lion builders? IE: "SnowLeopard Intel Release (Tests)" would have its results sent to mac-snowleopard?
>
> > > > > 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.
> >
>
> Ouch. My head hurts here at introducing this concept. I think what you actually want is something closer to the layout_tests.layout_package.test_output concept, possibly combined with the expected_* methods on the Port class.
>
> Also, please don't call this "Result" - that is too generic and overloaded a term. Even if you were to call it a "TestResult", that's confusing, because is the object referring to the actual expected output, or the results of running the test and comparing against the expectations (e.g., "PASS"/"FAIL", etc.)?
Maybe we can use the term 'baseline' to mean the thing that this class represents, ie: something that the output of a test can be compared against to determine PASS/FAIL?
That seems to fit with the general phrase 'rebaselining' - it's taking baselines from builders and using them to overwrite our existing baselines. What do you think?
>
> I've been trying to morph the layout tests code into using something a bit more consistent, so we should line things up here if we can. I would prefer something like "TestOutput" here.
Could you be a bit more specific? How could I change Results to be more suitable?
>
> As to the overlap between test_output, test_types, and the port/* routines, those ned to be cleaned up as well. My current thinking is that the test_types classes are legacy objects from much earlier versions of new-run-webkit-tests (predating the port/* abstraction completely), and just need to go away. I.e., it's an abstraction that is confusing things.
>
> In the very near future we'll need to add support for audio files and tests, and this aspect of the design should really be cleaned up (I'm expecting by me) before then.
>
> Also, the work we're doing with reftests will impact this as well, so it would be good to get up to speed on that if you aren't already (and I've cc'ed Ito-san on this to make sure he's aware of it as well).
I'm not familiar with reftests. Could you point me to somewhere I could learn more about them?
--
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