Added support for GPU-based expectations

Added support for GPU-based expectations

BUG=245741
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218650

[bajones, 2013-08-15 17:33:47]

Uses gpu information exposed by https://chromiumcodereview.appspot.com/21682002/
to allow setting of test expectations based on GPU vendor.

[Ken Russell (switch to Gerrit), 2013-08-15 19:25:28]

Good start but needs some more work.

https://codereview.chromium.org/22909011/diff/1/content/test/gpu/gpu_tests/we...
File content/test/gpu/gpu_tests/webgl_conformance.py (right):

https://codereview.chromium.org/22909011/diff/1/content/test/gpu/gpu_tests/we...
content/test/gpu/gpu_tests/webgl_conformance.py:64:
options.AppendExtraBrowserArg('--enable-gpu-benchmarking')
This argument isn't necessary in order to use the new SystemInfo and GPUInfo
classes.

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/page_runner.py:186: expectation =
expectations.GetExpectationForPage(state, page)
Passing the unspecified "state" object is sloppy. Pass state.browser instead.

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/test_expectations.py (right):

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/test_expectations.py:96: # TODO: Add checks
against other modifiers (configuration, etc.)
Rather than delaying the work, please pull in all the functionality from the
gpu_test_expectations_parser and webgl_conformance_test_expectations.txt -- at
least the PCI device ID, if DEBUG and RELEASE aren't feasible to implement yet.

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/test_expectations_unittest.py (right):

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/test_expectations_unittest.py:24: self.vendor_id
= 0
Rather than defining StubGpu, StubGpuInfo and StubSystemInfo, please use
SystemInfo.FromDict, so that more of the real code is tested.

https://codereview.chromium.org/22909011/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/test_expectations_unittest.py:134: def
testOSAndGpuExpectations(self):
The new tests look good, but assuming the PCI ID is incorporated into the test
expectations, then please expand upon the tests.

[bajones, 2013-08-16 23:01:54]

I don't see any way to handle DEBUG/RELEASE at the moment, it will probably have
to be plumbed through from the browser. Everything else has been addressed.

[Zhenyao Mo, 2013-08-16 23:16:36]

https://codereview.chromium.org/22909011/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/test_expectations.py (right):

https://codereview.chromium.org/22909011/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/test_expectations.py:108:
self._GetGpuDeviceId(gpu_info) in expectation.device_id_conditions)
this should add "and expectation.gpu_conditions and gpu_matches"

[Zhenyao Mo, 2013-08-16 23:41:05]

On 2013/08/16 23:16:36, Zhenyao Mo wrote:
>
https://codereview.chromium.org/22909011/diff/8001/tools/telemetry/telemetry/...
> File tools/telemetry/telemetry/page/test_expectations.py (right):
> 
>
https://codereview.chromium.org/22909011/diff/8001/tools/telemetry/telemetry/...
> tools/telemetry/telemetry/page/test_expectations.py:108:
> self._GetGpuDeviceId(gpu_info) in expectation.device_id_conditions)
> this should add "and expectation.gpu_conditions and gpu_matches"

LGTM

[dtu, 2013-08-17 00:26:27]

https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/test_expectations.py (right):

https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/test_expectations.py:30: if isinstance(c, (int,
long)):
if isinstance(c, numbers.Integral)

Also this worries me. Is there never going to be another integral expectation
condition that could conflict here?

[nduca, 2013-08-17 00:31:15]

If we have failures that are DEBUG/RELEASE specific then this is a problem for
telemetry. The buildtype is sometign we try to hide --- for good reason --- is a
profile build aother build type? is official build? what about an instrumented
build with valgrind? This gets hairy quickly... ideally, the suppressions we
allow here are really the ones that make sense as platform level failures... if
we have failures that are buildtype specific, i would much rather the whole test
be marked failing, the patch reverted, etc.

[bajones, 2013-08-19 16:59:21]

On 2013/08/17 00:26:27, Dave Tu wrote:
>
https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
> File tools/telemetry/telemetry/page/test_expectations.py (right):
> 
>
https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
> tools/telemetry/telemetry/page/test_expectations.py:30: if isinstance(c, (int,
> long)):
> if isinstance(c, numbers.Integral)
> 
> Also this worries me. Is there never going to be another integral expectation
> condition that could conflict here?

This logic was adopted from the existing exceptions logic. (See comments at the
top of
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...)

I wouldn't be opposed to making this more explicit, but I'm wary of introducing
unnecessary complexity. ie: If we can do this as ["nvidia", gpu(0x1002),
gpu(0x1003)] I think that's a totally reasonable compromise. I'd be reluctant to
turn it into test_expectations.gpu_device_id(0x1002), though. Readability of
these files is going to be critical for gardeners/sheriffs moving forward.

[dtu, 2013-08-20 05:41:10]

On 2013/08/19 16:59:21, bajones wrote:
> On 2013/08/17 00:26:27, Dave Tu wrote:
> >
>
https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
> > File tools/telemetry/telemetry/page/test_expectations.py (right):
> > 
> >
>
https://codereview.chromium.org/22909011/diff/14001/tools/telemetry/telemetry...
> > tools/telemetry/telemetry/page/test_expectations.py:30: if isinstance(c,
(int,
> > long)):
> > if isinstance(c, numbers.Integral)
> > 
> > Also this worries me. Is there never going to be another integral
expectation
> > condition that could conflict here?
> 
> This logic was adopted from the existing exceptions logic. (See comments at
the
> top of
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...)
> 
> I wouldn't be opposed to making this more explicit, but I'm wary of
introducing
> unnecessary complexity. ie: If we can do this as ["nvidia", gpu(0x1002),
> gpu(0x1003)] I think that's a totally reasonable compromise. I'd be reluctant
to
> turn it into test_expectations.gpu_device_id(0x1002), though. Readability of
> these files is going to be critical for gardeners/sheriffs moving forward.

How about a tuple? Since it has to be paired with a vendor anyway. ['mac',
('amd', 0x1234)]

[bajones, 2013-08-20 19:32:50]

On 2013/08/20 05:41:10, Dave Tu wrote:
> How about a tuple? Since it has to be paired with a vendor anyway. ['mac',
> ('amd', 0x1234)]

I really like this idea. Done!

[Zhenyao Mo, 2013-08-20 20:27:27]

On 2013/08/20 19:32:50, bajones wrote:
> On 2013/08/20 05:41:10, Dave Tu wrote:
> > How about a tuple? Since it has to be paired with a vendor anyway. ['mac',
> > ('amd', 0x1234)]
> 
> I really like this idea. Done!

I think the current implementation allows you to specify say [lion,
mountainlion, amd, 0x1234, 0x5678], i.e., making it easier to specify a group of
situations.

If you use pairs or tuples, then we have to make multiple entries.  Which I
guess is OK too.

Just want to raise this to you.

[Ken Russell (switch to Gerrit), 2013-08-20 20:48:43]

LGTM. One comment regarding the test structure; not necessary to make a change
in response.

https://codereview.chromium.org/22909011/diff/23001/content/test/gpu/gpu_test...
File content/test/gpu/gpu_tests/webgl_conformance_expectations.py (right):

https://codereview.chromium.org/22909011/diff/23001/content/test/gpu/gpu_test...
content/test/gpu/gpu_tests/webgl_conformance_expectations.py:13: # Example:
('nvidia, 0x1234)
Missing close quote.

https://codereview.chromium.org/22909011/diff/23001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/test_expectations_unittest.py (right):

https://codereview.chromium.org/22909011/diff/23001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/test_expectations_unittest.py:30:
'vendor_string': gpu, 'device_string': '' },
It's unusual that the GPU information will come back from the browser in this
configuration, where the vendor ID is zero, the device ID is nonzero, and the
vendor string encodes the name. It would be more realistic to define variables
with the hex codes for the vendor IDs you intend to test.

[bajones, 2013-08-20 21:42:46]

On 2013/08/20 20:48:43, Ken Russell wrote:
> LGTM. One comment regarding the test structure; not necessary to make a change
> in response.

Changed it anyway, might as well make the test at "realistic" as possible, plus
this hits more of the live codepath.

@dtu, any further comments?

[dtu, 2013-08-20 23:28:25]

lgtm

[commit-bot: I haz the power, 2013-08-20 23:38:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bajones@chromium.org/22909011/30001

[commit-bot: I haz the power, 2013-08-21 04:59:09]

Change committed as 218650