Bug 55191 - rebaseline-chromium-webkit-tests doesn't handle GPU modifiers "correctly"
Summary: rebaseline-chromium-webkit-tests doesn't handle GPU modifiers "correctly"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on: 55608
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-24 16:02 PST by Dirk Pranke
Modified: 2011-04-01 12:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (48.89 KB, patch)
2011-03-31 18:55 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (49.30 KB, patch)
2011-03-31 19:00 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
update w/ review feedback from tony (52.24 KB, patch)
2011-04-01 11:50 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
more feedback (52.22 KB, patch)
2011-04-01 12:06 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-02-24 16:02:17 PST
See bug 55002 for some background. Right now you have to specify "-g" or not to rebaseline-chromium-webkit-tests in order to get GPU baselines instead of the regular baselines. It would be much better if the script just matched against the "GPU" modifier and knew whether to pull from the GPU results or the CPU results.
Comment 1 Dirk Pranke 2011-03-31 18:55:18 PDT
Created attachment 87816 [details]
Patch
Comment 2 Dirk Pranke 2011-03-31 18:58:04 PDT
This change plus the fix in 55608 should make the rebaselining tool correctly again, in fact, probably better than it ever has before. 

Note that this removes the -g and -w flags; it *only* works on the canaries now, since that's the only place we have a consistent set of bots and because it's the only place rebaselining makes sense yet.

We can change this to work w/ build.webkit.org if/when we get a full set of bots there.

The -g flag is gone because GPU and CPU baselines are both handled in a single pass now.
Comment 3 Dirk Pranke 2011-03-31 19:00:59 PDT
Created attachment 87817 [details]
Patch
Comment 4 Tony Chang 2011-04-01 10:29:37 PDT
Comment on attachment 87817 [details]
Patch

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

In general looks OK, mostly some questions and a rebase is probably in order.

> Tools/ChangeLog:13
> +        actually rebaselined will be deleted, even if only one of the
> +        variants was actually rebaselined. This may cause odd problems,

Is this still the case?  Are there changes for this patch after rebaselining against 55608?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:443
> +        for line in self._get_iterable_expectations(self._expectations):
> +            lineno += 1

Can you use enumerate() instead of counting manually?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:93
> +        if not hasattr(self, '_operating_system'):

Is it possible for to always set these here and the sub classes can overwrite them after calling Port.__init__?  It would also encourage calling Port.__init__ as soon as possible (which is more like how C++ works).  This is mainly confusing in ChromiumLinux.__init__ where _version is set before and _architecture is set afterwards.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:-816
> -    option_parser.add_option('-w', '--webkit_canary',
> -                             action='store_true',
> -                             default=False,
> -                             help=('If True, pull baselines from webkit.org '
> -                                   'canary bot.'))

Do we want to keep the flag and have it just tell the user that this flag no longer does anything?  Might be confusing if someone tries to pass -w and they just get an error.  Alternately, we may just want to mention in --help that we always pull from the canaries.  Also, this could be a separate change.

> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:132
> +    d = {}

Please use a more descriptive variable name.
Comment 5 Dirk Pranke 2011-04-01 11:48:34 PDT
Comment on attachment 87817 [details]
Patch

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

>> Tools/ChangeLog:13
>> +        variants was actually rebaselined. This may cause odd problems,
> 
> Is this still the case?  Are there changes for this patch after rebaselining against 55608?

Yes, and no. This patch is baselined against 55608.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:443
>> +            lineno += 1
> 
> Can you use enumerate() instead of counting manually?

I didn't know about enumerate. Yes, that should work nicely.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:93

> 
> Is it possible for to always set these here and the sub classes can overwrite them after calling Port.__init__?  It would also encourage calling Port.__init__ as soon as possible (which is more like how C++ works).  This is mainly confusing in ChromiumLinux.__init__ where _version is set before and _architecture is set afterwards.

I've shuffled it around a bit. Unfortunately the responsibilities and contracts of the Port.__init__() constructor are pretty unclear and so the code is kinda ugly no matter which way you slice it. I have a separate set of patches coming that should clean all of this stuff up.

>> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py:-816
>> -    option_parser.add_option('-w', '--webkit_canary',
>> -                             action='store_true',
>> -                             default=False,
>> -                             help=('If True, pull baselines from webkit.org '
>> -                                   'canary bot.'))
> 
> Do we want to keep the flag and have it just tell the user that this flag no longer does anything?  Might be confusing if someone tries to pass -w and they just get an error.  Alternately, we may just want to mention in --help that we always pull from the canaries.  Also, this could be a separate change.

I can make this return a warning instead.

>> Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests_unittest.py:132
>> +    d = {}
> 
> Please use a more descriptive variable name.

done.
Comment 6 Dirk Pranke 2011-04-01 11:50:49 PDT
Created attachment 87891 [details]
update w/ review feedback from tony
Comment 7 Tony Chang 2011-04-01 11:58:01 PDT
Comment on attachment 87891 [details]
update w/ review feedback from tony

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

> Tools/ChangeLog:16
> +        This change removes the -g and -w flags. The -g flag is gone

Nit: removes -w to deprecates -w.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:443
> +        lineno = 0
> +        lines = []
> +        for line in self._get_iterable_expectations(self._expectations):
> +            lineno += 1

Nit: enumerate()
Comment 8 Dirk Pranke 2011-04-01 12:06:37 PDT
Created attachment 87893 [details]
more feedback
Comment 9 Dirk Pranke 2011-04-01 12:07:10 PDT
(In reply to comment #7)
> (From update of attachment 87891 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87891&action=review
> 
> > Tools/ChangeLog:16
> > +        This change removes the -g and -w flags. The -g flag is gone
> 
> Nit: removes -w to deprecates -w.
> 

Done.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:443
> > +        lineno = 0
> > +        lines = []
> > +        for line in self._get_iterable_expectations(self._expectations):
> > +            lineno += 1
> 
> Nit: enumerate()

Whoops :(. Done.
Comment 10 Dirk Pranke 2011-04-01 12:22:58 PDT
Committed r82705: <http://trac.webkit.org/changeset/82705>