Wire FailureValue in PageTestResults (try 2).

Wire FailureValue in PageTestResults (try 2).

Previous patch failed because of GC issue (GC for the first test
only happens after the second test started, and it turns out
that we rely on __del__ in that test -- not good!). This is fixed in:

https://codereview.chromium.org/409113006/

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

[nednguyen, 2014-07-22 21:59:34]

On 2014/07/22 21:58:20, chrishenry wrote:

Can you pull a power_metric fix to a different patch & add unittest for it?

[chrishenry, 2014-07-22 23:16:01]

On 2014/07/22 21:59:34, nednguyen wrote:
> On 2014/07/22 21:58:20, chrishenry wrote:
> 
> Can you pull a power_metric fix to a different patch & add unittest for it?

Done. Will submit the other patch first before this.

[chrishenry, 2014-07-23 14:13:58]

On 2014/07/22 23:16:01, chrishenry wrote:
> On 2014/07/22 21:59:34, nednguyen wrote:
> > On 2014/07/22 21:58:20, chrishenry wrote:
> > 
> > Can you pull a power_metric fix to a different patch & add unittest for it?
> 
> Done. Will submit the other patch first before this.

Ok, rebased. PTAL. Thanks!

[nednguyen, 2014-07-23 16:34:49]

https://codereview.chromium.org/408913002/diff/80001/tools/perf/measurements/...
File tools/perf/measurements/skpicture_printer_unittest.py (right):

https://codereview.chromium.org/408913002/diff/80001/tools/perf/measurements/...
tools/perf/measurements/skpicture_printer_unittest.py:32: assert 'not supported'
in failure.GetStringFromExcInfo(
A better alternative could be str(results.failures[0]) that return
_GetStringFromExcInfo(..) or I think results.failures[0][1].message would be
enough.

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results.py (right):

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:46: pages =
set([value.page for value in self._all_page_specific_values])
nits:  
There is no need to allocate a list here and in code below too.

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:53: return set([v.page
for v in self.failures])
"Accordingly, the requirements for set elements are the same as those for
dictionary keys; namely, that the element defines both __eq__() and __hash__().
As a result, sets cannot contain mutable elements such as lists or
dictionaries."

https://docs.python.org/2/library/sets.html

I am pretty sure that our page.Page class doesn't satisfy these requirements
strictly.

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:60: def
_GetStringFromExcInfo(self, err):
Dead code.

[chrishenry, 2014-07-23 17:48:25]

https://codereview.chromium.org/408913002/diff/80001/tools/perf/measurements/...
File tools/perf/measurements/skpicture_printer_unittest.py (right):

https://codereview.chromium.org/408913002/diff/80001/tools/perf/measurements/...
tools/perf/measurements/skpicture_printer_unittest.py:32: assert 'not supported'
in failure.GetStringFromExcInfo(
On 2014/07/23 16:34:49, nednguyen wrote:
> A better alternative could be str(results.failures[0]) that return
> _GetStringFromExcInfo(..) or I think results.failures[0][1].message would be
> enough.

Done. results.failures[0].exc_info[1].message is the one you want.

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/results/page_test_results.py (right):

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:46: pages =
set([value.page for value in self._all_page_specific_values])
On 2014/07/23 16:34:49, nednguyen wrote:
> nits:  
> There is no need to allocate a list here and in code below too.

Done.

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:53: return set([v.page
for v in self.failures])
On 2014/07/23 16:34:49, nednguyen wrote:
> "Accordingly, the requirements for set elements are the same as those for
> dictionary keys; namely, that the element defines both __eq__() and
__hash__().
> As a result, sets cannot contain mutable elements such as lists or
> dictionaries."
> 
> https://docs.python.org/2/library/sets.html
> 
> I am pretty sure that our page.Page class doesn't satisfy these requirements
> strictly.

This is fine, see
https://docs.python.org/2/glossary.html#term-hashable

In particular: "Objects which are instances of user-defined classes are hashable
by default; they all compare unequal (except with themselves), and their hash
value is their id()."

https://codereview.chromium.org/408913002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/results/page_test_results.py:60: def
_GetStringFromExcInfo(self, err):
On 2014/07/23 16:34:49, nednguyen wrote:
> Dead code.

Done.

[nednguyen, 2014-07-23 17:52:01]

lgtm

[chrishenry, 2014-07-23 21:20:26]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-07-23 21:22:01]

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

[commit-bot: I haz the power, 2014-07-23 23:02:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)

[commit-bot: I haz the power, 2014-07-23 23:54:51]

Change committed as 285078