Bug 88581 – webkit-patch rebaseline-expectations should only rebaseline the appropriate suffixes for the failure in question

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

currently rebaseline-expectations will rebaseline 'txt' files even if the failure is only listed as IMAGE. This can result in stale files getting pulled from the bot. We need to limit to the appropriate suffixes just like we do in garden-o-matic.

See bug 88561 for one instance where this wreaked havoc.

[Dirk Pranke]

Created attachment 146399 [details]
Patch

[Ojan Vafai]

Comment on attachment 146399 [details]
Patch

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

Thanks for the quick fix!

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:88
> +# FIXME: Perhas these two routines should be part of the Port instead?

s/Perhas/Perhaps. I doubt these will end up being port-specific. The platform identifies might though. Either way, no harm in having a FIXME.

> Tools/Scripts/webkitpy/tool/commands/rebaseline.py:289
> +            self._run_webkit_patch(['rebaseline-test', '--suffixes', ','.join(suffixes), builder_name, test_name])

Maybe add a FIXME to use run_in_parallel here?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 146399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146399&action=review
> 
> Thanks for the quick fix!
> 
> > Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:88
> > +# FIXME: Perhas these two routines should be part of the Port instead?
> 
> s/Perhas/Perhaps. I doubt these will end up being port-specific. The platform identifies might though. Either way, no harm in having a FIXME.
> 

Yeah.

> > Tools/Scripts/webkitpy/tool/commands/rebaseline.py:289
> > +            self._run_webkit_patch(['rebaseline-test', '--suffixes', ','.join(suffixes), builder_name, test_name])
> 
> Maybe add a FIXME to use run_in_parallel here?

Oh, good idea.

[Dirk Pranke]

Committed r119766: <http://trac.webkit.org/changeset/119766>

[Elliot Poger]

*** Bug 88561 has been marked as a duplicate of this bug. ***

[Elliot Poger]

Created attachment 147655 [details]
command-lines I ran to rebaseline a single image test

I don't think this bug is fixed.

I tried to rebaseline a single IMAGE failure (on Windows only) using webkit-patch rebaseline-expectations, and I ended up with all these changes: 

$ svn status
A       LayoutTests/platform/qt/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/gtk/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/mac/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/chromium-mac-snowleopard/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/chromium-mac/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/chromium-mac-leopard/fast/gradients/background-clipped-expected.txt
D       LayoutTests/platform/efl/fast/gradients/background-clipped-expected.txt
M       LayoutTests/platform/chromium-win/fast/gradients/background-clipped-expected.png
R       LayoutTests/fast/gradients/background-clipped-expected.txt

Much more detail in the attached log.

[Elliot Poger]

See above comment/attachment.  Am I doing something wrong?

[Ojan Vafai]

This is not necessarily a bug. Rebaseline-expectations rebaselines and then calls optimize-baselines. If you just call optimize-baselines for that test on a clean tree do you get the same text file changes? If so, then it's probably still correct.

[Dirk Pranke]

(In reply to comment #8)
> This is not necessarily a bug. Rebaseline-expectations rebaselines and then calls optimize-baselines. If you just call optimize-baselines for that test on a clean tree do you get the same text file changes? If so, then it's probably still correct.

Ojan is correct that we're optimizing everything even though we only rebaselined the IMAGE, and optimize is working as expected. rebaseline-expectations isn't actually passing the appropriate suffix list to optimize, though, and it probably should, just to reduce confusion.

[Dirk Pranke]

Created attachment 147673 [details]
Patch

[Elliot Poger]

Created attachment 147807 [details]
log of re-running previous commands with patch in place

Yup, that patch fixes it!  (See attached output.)  Thanks!  I will hit commit+ ...

[WebKit Review Bot]

Comment on attachment 147673 [details]
Patch

Clearing flags on attachment: 147673

Committed r120454: <http://trac.webkit.org/changeset/120454>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.