Wire FailureValue in PageTestResults (try 2).

tools/telemetry/telemetry/results/page_test_results.py

Index: tools/telemetry/telemetry/results/page_test_results.py
diff --git a/tools/telemetry/telemetry/results/page_test_results.py b/tools/telemetry/telemetry/results/page_test_results.py
index bd76ce422b07051e7415f03638c579b3d2dd148a..5bab580fd53c256dfa3bda4ee7e1ea339d6b7aac 100644
--- a/tools/telemetry/telemetry/results/page_test_results.py
+++ b/tools/telemetry/telemetry/results/page_test_results.py
@@ -5,18 +5,18 @@
 import collections
 import copy
 import logging
-import sys
 import traceback
 
 from telemetry import value as value_module
+from telemetry.value import failure
 
 class PageTestResults(object):
   def __init__(self, output_stream=None):
     super(PageTestResults, self).__init__()
     self._output_stream = output_stream
-    self.pages_that_had_failures = set()
+    # TODO(chrishenry,eakuefner): Remove self.successes once they can
+    # be inferred.
     self.successes = []
-    self.failures = []
     self.skipped = []
 
     self._representative_value_for_each_value_name = {}
@@ -42,10 +42,21 @@ class PageTestResults(object):
 
   @property
   def pages_that_succeeded(self):
+    """Returns the set of pages that succeeded."""
     pages = set([value.page for value in self._all_page_specific_values])

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

nits:  
There is no need to allocate a list here and in code below too.

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

Done.

     pages.difference_update(self.pages_that_had_failures)
     return pages
 
+  @property
+  def pages_that_had_failures(self):
+    """Returns the set of failed pages."""
+    return set([v.page for v in self.failures])

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

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

[chrishenry, 2014/07/23 17:48:24]

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

+
+  @property
+  def failures(self):
+    values = self._all_page_specific_values
+    return [v for v in values if isinstance(v, failure.FailureValue)]
+
   def _GetStringFromExcInfo(self, err):

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

Dead code.

[chrishenry, 2014/07/23 17:48:24]

Done.

     return ''.join(traceback.format_exception(*err))
 
@@ -72,26 +83,16 @@ class PageTestResults(object):
         value.name]
     assert value.IsMergableWith(representative_value)
 
-  def AddFailure(self, page, err):
-    self.pages_that_had_failures.add(page)
-    self.failures.append((page, self._GetStringFromExcInfo(err)))
-
   def AddSkip(self, page, reason):
     self.skipped.append((page, reason))
 
   def AddSuccess(self, page):
     self.successes.append(page)
 
-  def AddFailureMessage(self, page, message):
-    try:
-      raise Exception(message)
-    except Exception:
-      self.AddFailure(page, sys.exc_info())
-
   def PrintSummary(self):
     if self.failures:
       logging.error('Failed pages:\n%s', '\n'.join(
-          p.display_name for p in zip(*self.failures)[0]))
+          p.display_name for p in self.pages_that_had_failures))
 
     if self.skipped:
       logging.warning('Skipped pages:\n%s', '\n'.join(