Bug 55191 – rebaseline-chromium-webkit-tests doesn't handle GPU modifiers "correctly"

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 87816 [details]
Patch

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 87817 [details]
Patch

[Tony Chang]

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.

[Dirk Pranke]

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.

[Dirk Pranke]

Created attachment 87891 [details]
update w/ review feedback from tony

[Tony Chang]

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()

[Dirk Pranke]

Created attachment 87893 [details]
more feedback

[Dirk Pranke]

(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.

[Dirk Pranke]

Committed r82705: <http://trac.webkit.org/changeset/82705>