[Telemetry] Print summaries for individual passing pages if failed pages exist.

[Telemetry] Print summaries for individual passing pages if failed pages exist.

Previously, Telemetry would not output any summary ("RESULT=") lines if
at least one page in a page set failed.  Now, if a failed page exists,
Telemetry will output the individual page results from the passing pages,
but it will not output any average data or any overall results that are
not associated with a page.

Also added a few unit tests for this change.

BUG=230998
TEST=Verified with the scrolling_benchmark on a local chromeOS device
(which currently has at least one failing page) that RESULT= lines are
now outputted only for the passing pages.  Also verified that all unit
tests for page_benchmark_results.py pass.

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

[tonyg, 2013-04-16 21:31:37]

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/page_benchmark_results.py (right):

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/page_benchmark_results.py:176:
(self.page_failures and len(value_url_list) == 1)):
I don't understand why this condition was added. Can you explain? Do the new
test cases cover it?

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/page_benchmark_results_unittest.py (right):

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/page_benchmark_results_unittest.py:146: def
test_repeated_pageset(self):
Can you also add a test with a repeated page_set like this one, except that it
has a failure on just one of the iterations of one of the URLs. I'd expect it to
only have a _by_url result for the page with no failures.

[dennis_jeffrey, 2013-04-16 23:16:30]

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/page_benchmark_results.py (right):

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/page_benchmark_results.py:176:
(self.page_failures and len(value_url_list) == 1)):
On 2013/04/16 21:31:37, tonyg wrote:
> I don't understand why this condition was added. Can you explain? Do the new
> test cases cover it?

|value_url_list| will only contain results for successfully-completed pages.  In
the event that multiple pages were run but only a single page was successful, we
should still have the "_by_url" result for that page printed like before. 
Without adding this condition, we would either:

1) Not output any results, given the check I added at line 192 below, or

2) Without adding the check at line 192 below, we'd output the single successful
page result as a "non-_by_url" result line, rather than a "_by_url" result line.

A new test case I had added in this CL ("test_basic_summary_pass_and_fail_page")
covers this case, because it involves 1 passing and 1 failing page.  In fact,
when I first wrote that test, it failed, and that's what prompted me to add this
condition.

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
File tools/telemetry/telemetry/page/page_benchmark_results_unittest.py (right):

https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
tools/telemetry/telemetry/page/page_benchmark_results_unittest.py:146: def
test_repeated_pageset(self):
On 2013/04/16 21:31:37, tonyg wrote:
> Can you also add a test with a repeated page_set like this one, except that it
> has a failure on just one of the iterations of one of the URLs. I'd expect it
to
> only have a _by_url result for the page with no failures.

Done.

[tonyg, 2013-04-16 23:55:58]

On 2013/04/16 23:16:30, dennis_jeffrey wrote:
>
https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
> File tools/telemetry/telemetry/page/page_benchmark_results.py (right):
> 
>
https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
> tools/telemetry/telemetry/page/page_benchmark_results.py:176:
> (self.page_failures and len(value_url_list) == 1)):
> On 2013/04/16 21:31:37, tonyg wrote:
> > I don't understand why this condition was added. Can you explain? Do the new
> > test cases cover it?
> 
> |value_url_list| will only contain results for successfully-completed pages. 
In
> the event that multiple pages were run but only a single page was successful,
we
> should still have the "_by_url" result for that page printed like before. 
> Without adding this condition, we would either:
> 
> 1) Not output any results, given the check I added at line 192 below, or
> 
> 2) Without adding the check at line 192 below, we'd output the single
successful
> page result as a "non-_by_url" result line, rather than a "_by_url" result
line.
> 
> A new test case I had added in this CL
("test_basic_summary_pass_and_fail_page")
> covers this case, because it involves 1 passing and 1 failing page.  In fact,
> when I first wrote that test, it failed, and that's what prompted me to add
this
> condition.

Ah, I follow now. Thanks for the explanation.

lgtm

> 
>
https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
> File tools/telemetry/telemetry/page/page_benchmark_results_unittest.py
(right):
> 
>
https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/teleme...
> tools/telemetry/telemetry/page/page_benchmark_results_unittest.py:146: def
> test_repeated_pageset(self):
> On 2013/04/16 21:31:37, tonyg wrote:
> > Can you also add a test with a repeated page_set like this one, except that
it
> > has a failure on just one of the iterations of one of the URLs. I'd expect
it
> to
> > only have a _by_url result for the page with no failures.
> 
> Done.

[dennisjeffrey, 2013-04-16 23:58:22]

Thanks for the review!  Will submit this now.


On Tue, Apr 16, 2013 at 4:55 PM, <tonyg@chromium.org> wrote:

> On 2013/04/16 23:16:30, dennis_jeffrey wrote:
>
> https://chromiumcodereview.**appspot.com/14172017/diff/1/**
>
tools/telemetry/telemetry/**page/page_benchmark_results.py<https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/telemetry/page/page_benchmark_results.py>
>
>> File tools/telemetry/telemetry/**page/page_benchmark_results.py (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/14172017/diff/1/**
>
tools/telemetry/telemetry/**page/page_benchmark_results.**py#newcode176<https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/telemetry/page/page_benchmark_results.py#newcode176>
>
>> tools/telemetry/telemetry/**page/page_benchmark_results.**py:176:
>> (self.page_failures and len(value_url_list) == 1)):
>> On 2013/04/16 21:31:37, tonyg wrote:
>> > I don't understand why this condition was added. Can you explain? Do
>> the new
>> > test cases cover it?
>>
>
>  |value_url_list| will only contain results for successfully-completed
>> pages.
>>
> In
>
>> the event that multiple pages were run but only a single page was
>> successful,
>>
> we
>
>> should still have the "_by_url" result for that page printed like before.
>> Without adding this condition, we would either:
>>
>
>  1) Not output any results, given the check I added at line 192 below, or
>>
>
>  2) Without adding the check at line 192 below, we'd output the single
>>
> successful
>
>> page result as a "non-_by_url" result line, rather than a "_by_url" result
>>
> line.
>
>  A new test case I had added in this CL
>>
> ("test_basic_summary_pass_and_**fail_page")
>
>> covers this case, because it involves 1 passing and 1 failing page.  In
>> fact,
>> when I first wrote that test, it failed, and that's what prompted me to
>> add
>>
> this
>
>> condition.
>>
>
> Ah, I follow now. Thanks for the explanation.
>
> lgtm
>
>
>
>
> https://chromiumcodereview.**appspot.com/14172017/diff/1/**
>
tools/telemetry/telemetry/**page/page_benchmark_results_**unittest.py<https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/telemetry/page/page_benchmark_results_unittest.py>
>
>> File tools/telemetry/telemetry/**page/page_benchmark_results_**
>> unittest.py
>>
> (right):
>
>
> https://chromiumcodereview.**appspot.com/14172017/diff/1/**
> tools/telemetry/telemetry/**page/page_benchmark_results_**
>
unittest.py#newcode146<https://chromiumcodereview.appspot.com/14172017/diff/1/tools/telemetry/telemetry/page/page_benchmark_results_unittest.py#newcode146>
>
>> tools/telemetry/telemetry/**page/page_benchmark_results_**unittest.py:146:
>> def
>> test_repeated_pageset(self):
>> On 2013/04/16 21:31:37, tonyg wrote:
>> > Can you also add a test with a repeated page_set like this one, except
>> that
>>
> it
>
>> > has a failure on just one of the iterations of one of the URLs. I'd
>> expect
>>
> it
>
>> to
>> > only have a _by_url result for the page with no failures.
>>
>
>  Done.
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/14172017/<https://chromiumcodereview...
>

[dennis_jeffrey, 2013-04-17 00:06:28]

Committed patchset #2 manually as r194491 (presubmit successful).