[Telemetry] Modify ValueCanBeAddedPredicate so it cannot filter out skip & failure value.

In crbug.com/466196, the gpu_times benchmark unintentionally filters out skip
value, and made it hard to figure out what went wrong. When people do whitelist
filtering, most of them will also make the mistake of filtering our skip &
failure values unintentionally. I also think that these values shouldn't be
filtered out in any situation.

BUG=466196

Review URL: https://codereview.chromium.org/1011893002

Cr-Commit-Position: refs/heads/master@{#320871}

tools/telemetry/telemetry/results/page_test_results.py

@@ -37,9 +37,10 @@ class PageTestResults(object):
       output_dir: A string specified the directory where to store the test
           artifacts, e.g: trace, videos,...
       value_can_be_added_predicate: A function that takes two arguments:
-          a value.Value instance and a boolean (True when the value is part
-          of the first result for the user story). It returns True if the value
-          can be added to the test results and False otherwise.
+          a value.Value instance (except value.FailureValue & value.SkipValue)
+          and a boolean (True when the value is part of the first result for
+          the user story). It returns True if the value can be added to the
+          test results and False otherwise.
     """
     # TODO(chrishenry): Figure out if trace_tag is still necessary.
 
@@ -171,7 +172,9 @@ class PageTestResults(object):
     self._ValidateValue(value)
     is_first_result = (
       self._current_page_run.user_story not in self._all_user_stories)
-    if not self._value_can_be_added_predicate(value, is_first_result):
+    if not (isinstance(value, skip.SkipValue) or
+            isinstance(value, failure.FailureValue) or
+            self._value_can_be_added_predicate(value, is_first_result)):
       return
     # TODO(eakuefner/chrishenry): Add only one skip per pagerun assert here
     self._current_page_run.AddValue(value)

tools/telemetry/telemetry/results/page_test_results_unittest.py

@@ -300,3 +300,37 @@ class PageTestResultsFilterTest(unittest.TestCase):
     actual_values = [(v.name, v.page.url, v.value)
                      for v in results.all_page_specific_values]
     self.assertEquals(expected_values, actual_values)
+
+  def testFailureValueCannotBeFiltered(self):
+    def AcceptValueNamed_a(value, _):
+      return value.name == 'a'
+    results = page_test_results.PageTestResults(
+        value_can_be_added_predicate=AcceptValueNamed_a)
+    results.WillRunPage(self.pages[0])
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 8))
+    failure_value = failure.FailureValue.FromMessage(self.pages[0], 'failure')
+    results.AddValue(failure_value)
+    results.DidRunPage(self.pages[0])
+    results.PrintSummary()
+
+    # Although predicate says only accept values named 'a', the failure value is
+    # added anyway.
+    self.assertEquals(len(results.all_page_specific_values), 1)
+    self.assertIn(failure_value, results.all_page_specific_values)
+
+  def testSkipValueCannotBeFiltered(self):
+    def AcceptValueNamed_a(value, _):
+      return value.name == 'a'
+    results = page_test_results.PageTestResults(
+        value_can_be_added_predicate=AcceptValueNamed_a)
+    results.WillRunPage(self.pages[0])
+    skip_value = skip.SkipValue(self.pages[0], 'skip for testing')
+    results.AddValue(scalar.ScalarValue(self.pages[0], 'b', 'seconds', 8))
+    results.AddValue(skip_value)
+    results.DidRunPage(self.pages[0])
+    results.PrintSummary()
+
+    # Although predicate says only accept value with named 'a', skip value is
+    # added anyway.
+    self.assertEquals(len(results.all_page_specific_values), 1)
+    self.assertIn(skip_value, results.all_page_specific_values)