Bug 88581 - webkit-patch rebaseline-expectations should only rebaseline the appropriate suffixes for the failure in question
Summary: webkit-patch rebaseline-expectations should only rebaseline the appropriate s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
: 88561 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-06-07 15:10 PDT by Dirk Pranke
Modified: 2012-06-15 07:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (16.61 KB, patch)
2012-06-07 15:26 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
command-lines I ran to rebaseline a single image test (11.38 KB, text/plain)
2012-06-14 14:32 PDT, Elliot Poger
no flags Details
Patch (4.79 KB, patch)
2012-06-14 16:13 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
log of re-running previous commands with patch in place (3.98 KB, text/plain)
2012-06-15 07:15 PDT, Elliot Poger
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-06-07 15:10:44 PDT
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.
Comment 1 Dirk Pranke 2012-06-07 15:26:34 PDT
Created attachment 146399 [details]
Patch
Comment 2 Ojan Vafai 2012-06-07 15:36:54 PDT
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?
Comment 3 Dirk Pranke 2012-06-07 16:03:32 PDT
(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.
Comment 4 Dirk Pranke 2012-06-07 16:10:37 PDT
Committed r119766: <http://trac.webkit.org/changeset/119766>
Comment 5 Elliot Poger 2012-06-11 07:11:12 PDT
*** Bug 88561 has been marked as a duplicate of this bug. ***
Comment 6 Elliot Poger 2012-06-14 14:32:16 PDT
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.
Comment 7 Elliot Poger 2012-06-14 14:33:03 PDT
See above comment/attachment.  Am I doing something wrong?
Comment 8 Ojan Vafai 2012-06-14 15:25:09 PDT
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.
Comment 9 Dirk Pranke 2012-06-14 16:00:09 PDT
(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.
Comment 10 Dirk Pranke 2012-06-14 16:13:48 PDT
Created attachment 147673 [details]
Patch
Comment 11 Elliot Poger 2012-06-15 07:15:31 PDT
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+ ...
Comment 12 WebKit Review Bot 2012-06-15 07:22:31 PDT
Comment on attachment 147673 [details]
Patch

Clearing flags on attachment: 147673

Committed r120454: <http://trac.webkit.org/changeset/120454>
Comment 13 WebKit Review Bot 2012-06-15 07:22:36 PDT
All reviewed patches have been landed.  Closing bug.