Add FailureValue and wire it through PageTestResults.

Add FailureValue and wire it through PageTestResults.

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

[nednguyen, 2014-07-18 02:43:52]

Nice patch! Looks good to me overall.

+kbr since he uses GtestTestResults.

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/failure.py (right):

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/failure.py:10: class
FailureValue(value_module.Value):
We may want to define __hash__ operator for this one so we can suppress similar
FailureValues for a same page.

[chrishenry, 2014-07-18 18:14:18]

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/value/failure.py (right):

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/value/failure.py:10: class
FailureValue(value_module.Value):
On 2014/07/18 02:43:51, nednguyen wrote:
> We may want to define __hash__ operator for this one so we can suppress
similar
> FailureValues for a same page.

Curious why do you want to de-dupe them? If there are two failures with the same
message in a single run, I think we want to keep both?

[nednguyen, 2014-07-18 18:47:37]

On 2014/07/18 18:14:18, chrishenry wrote:
>
https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/value/failure.py (right):
> 
>
https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/value/failure.py:10: class
> FailureValue(value_module.Value):
> On 2014/07/18 02:43:51, nednguyen wrote:
> > We may want to define __hash__ operator for this one so we can suppress
> similar
> > FailureValues for a same page.
> 
> Curious why do you want to de-dupe them? If there are two failures with the
same
> message in a single run, I think we want to keep both?

It was Nat's idea that we want to avoid spamming the log/output results with
these error mesaages. To avoid losing info, we can keep a counter to say "that
errors happen x time", maybe through the merge function.

[chrishenry, 2014-07-18 19:52:15]

On 2014/07/18 18:47:37, nednguyen wrote:
> On 2014/07/18 18:14:18, chrishenry wrote:
> >
>
https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/value/failure.py (right):
> > 
> >
>
https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/value/failure.py:10: class
> > FailureValue(value_module.Value):
> > On 2014/07/18 02:43:51, nednguyen wrote:
> > > We may want to define __hash__ operator for this one so we can suppress
> > similar
> > > FailureValues for a same page.
> > 
> > Curious why do you want to de-dupe them? If there are two failures with the
> same
> > message in a single run, I think we want to keep both?
> 
> It was Nat's idea that we want to avoid spamming the log/output results with
> these error mesaages. To avoid losing info, we can keep a counter to say "that
> errors happen x time", maybe through the merge function.

Let's follow up on this as a separate issue/patch. I'm not convinced that
FailureValue is the right answer for that kind of logging spam.

For example, some of the log spam can be associated with a metric, and as such,
the right model might be to move this kind of warnings to the corresponding
values rather than as a separate logging - this can be surfaced better in the UI
as well.

[Ken Russell (switch to Gerrit), 2014-07-18 21:25:48]

LGTM as long as this passes the bots. It may be worth sending this to the try
bots via "git cl try" just in case it somehow breaks the error reporting in a
way that no failures are produced any more for gtest-related Telemetry tests.

One comment as an observer; I haven't been involved in the design here.

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/gtest_test_results.py (right):

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/gtest_test_results.py:29: 'GTestTestResults
only accepts FailureValue.')
It seems asymmetric to me that failures are represented as values but other
results (successes, skipped tests) aren't.

[chrishenry, 2014-07-18 22:05:52]

On 2014/07/18 21:25:48, Ken Russell wrote:
> LGTM as long as this passes the bots. It may be worth sending this to the try
> bots via "git cl try" just in case it somehow breaks the error reporting in a
> way that no failures are produced any more for gtest-related Telemetry tests.

OOC, what's different about git cl try? I was wondering about how to test the
case you just described too. How would git cl try help? When tested manually,
the output looks ok for both success and fail case, but you're right - with
changes like this (and this won't be the last one), I'm worried of misreporting
failures as successes.

[chrishenry, 2014-07-18 22:08:24]

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/gtest_test_results.py (right):

https://codereview.chromium.org/399263003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/gtest_test_results.py:29: 'GTestTestResults
only accepts FailureValue.')
On 2014/07/18 21:25:48, Ken Russell wrote:
> It seems asymmetric to me that failures are represented as values but other
> results (successes, skipped tests) aren't.

Skip will be represented as value too; eakuefner is looking into that portion.
We can infer success status based on the existence of failure values.

[Ken Russell (switch to Gerrit), 2014-07-18 22:50:15]

On 2014/07/18 22:05:52, chrishenry wrote:
> On 2014/07/18 21:25:48, Ken Russell wrote:
> > LGTM as long as this passes the bots. It may be worth sending this to the
try
> > bots via "git cl try" just in case it somehow breaks the error reporting in
a
> > way that no failures are produced any more for gtest-related Telemetry
tests.
> 
> OOC, what's different about git cl try? I was wondering about how to test the
> case you just described too. How would git cl try help? When tested manually,
> the output looks ok for both success and fail case, but you're right - with
> changes like this (and this won't be the last one), I'm worried of
misreporting
> failures as successes.

Actually what I was concerned about was a scenario in which the GPU bots stopped
reporting both successes and failures for some pathological reason. The unit
tests will probably prevent this from happening. git cl try just runs the same
bots that the commit queue would without committing the patch at the end,
letting you manually check e.g. the win_gpu_triggered_tests results to ensure
they're OK.

[chrishenry, 2014-07-21 17:04:25]

On 2014/07/18 22:50:15, Ken Russell wrote:
> On 2014/07/18 22:05:52, chrishenry wrote:
> > On 2014/07/18 21:25:48, Ken Russell wrote:
> > > LGTM as long as this passes the bots. It may be worth sending this to the
> try
> > > bots via "git cl try" just in case it somehow breaks the error reporting
in
> a
> > > way that no failures are produced any more for gtest-related Telemetry
> tests.
> > 
> > OOC, what's different about git cl try? I was wondering about how to test
the
> > case you just described too. How would git cl try help? When tested
manually,
> > the output looks ok for both success and fail case, but you're right - with
> > changes like this (and this won't be the last one), I'm worried of
> misreporting
> > failures as successes.
> 
> Actually what I was concerned about was a scenario in which the GPU bots
stopped
> reporting both successes and failures for some pathological reason. The unit
> tests will probably prevent this from happening. git cl try just runs the same
> bots that the commit queue would without committing the patch at the end,
> letting you manually check e.g. the win_gpu_triggered_tests results to ensure
> they're OK.

Ok, ran the try bots, they look ok.

[nednguyen, 2014-07-21 17:05:43]

lgtm

[chrishenry, 2014-07-21 17:18:25]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-07-21 17:19:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishenry@google.com/399263003/80001

[commit-bot: I haz the power, 2014-07-21 18:04:06]

Change committed as 284459