Rebaselining for a new port doesn't work right with multiple fallback ports

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Rebaselining for a new port doesn't work right with multiple fallback ports

[Ojan Vafai]

Created attachment 131499 [details]
Patch

[Adam Barth]

Comment on attachment 131499 [details]
Patch

It looks like you didn't add any tests for this new functionality.

[Adam Barth]

Comment on attachment 131499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131499&action=review

> Tools/ChangeLog:10
> +        Since chromium-leopard also falls back to mac-leopard, we need to
> +        copy the existing result for both chromium-leopard and chromium-snowleopard
> +        before doing lion rebaselines.

I see.  Soooo complicated.  At some point we should decide that being parasitic on apple-mac isn't worth the complexity.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:71
> +    def _copy_existing_baseline(self, platforms_to_move_existing_baselines_to, test_name, suffix):

This seems very likely to create non-optimized baselines.  Is there an optimization step to clean up after these moves?

Moving baselines around is pretty complicated.  That's why, in the other class, I went with the approach of preparing the whole move and then double-checking that I didn't break anything (by seeing that the before/after results were the same).  I wonder if an approach that would be useful here...

One last comment is that this command has grown out of control.  It really needs to be refactored into smaller interacting pieces rather than having all the logic in one giant class.  (That's perhaps a yak for another day, however.)

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 131499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131499&action=review
> 
> > Tools/ChangeLog:10
> > +        Since chromium-leopard also falls back to mac-leopard, we need to
> > +        copy the existing result for both chromium-leopard and chromium-snowleopard
> > +        before doing lion rebaselines.
> 
> I see.  Soooo complicated.  At some point we should decide that being parasitic on apple-mac isn't worth the complexity.

I was thinking it would be good to only ever fallback to platform/mac. That would make all the chromium ports' fallbacks form a tree. Would that make all the ports' fallbacks form a tree?

> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:71
> > +    def _copy_existing_baseline(self, platforms_to_move_existing_baselines_to, test_name, suffix):
> 
> This seems very likely to create non-optimized baselines.  Is there an optimization step to clean up after these moves?

webkit-patch optimize-baselines does. In practice, this is only ever called from garden-o-matic, which optimizes the baselines.

> Moving baselines around is pretty complicated.  That's why, in the other class, I went with the approach of preparing the whole move and then double-checking that I didn't break anything (by seeing that the before/after results were the same).  I wonder if an approach that would be useful here...
> 
> One last comment is that this command has grown out of control.  It really needs to be refactored into smaller interacting pieces rather than having all the logic in one giant class.  (That's perhaps a yak for another day, however.)

I'd rather not invest too much more effort into this code. It's only hit when bringing up a new port and hopefully we'll fix our fallback order to be more of a tree in the not too distant future.

> It looks like you didn't add any tests for this new functionality.

Whoops. Will add a test and then commit.

[Ojan Vafai]

Committed r110585: <http://trac.webkit.org/changeset/110585>

[Adam Barth]

> I was thinking it would be good to only ever fallback to platform/mac. That would make all the chromium ports' fallbacks form a tree. Would that make all the ports' fallbacks form a tree?

That's an interesting idea.  It might work well.  We wouldn't quite be left with a tree, but we'd be much, much closer (and could probably clean up the remaining non-tree paths).