Two unexpected pass finder improvements

Fixes/improves the GPU unexpected pass finder script in the following
ways:

1. Adds support for preventing expectations from being auto-removed via
   block or inline comments. This is mainly intended for cases where the
   flake rate for a test is low enough that the script does not reliably
   find a flake with a reasonable sample size.
2. Works around the suite name being reported to typ/ResultDB not being
   the same as the suite name reported by Telemetry for several suites.

Bug: 998329
Change-Id: Ie9119dbf24353de7487cb1601db8234fd8efcd90
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2590365
Auto-Submit: Brian Sheedy <bsheedy@chromium.org>
Commit-Queue: Yuly Novikov <ynovikov@chromium.org>
Reviewed-by: Yuly Novikov <ynovikov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836887}

content/test/gpu/gpu_tests/test_expectations/pixel_expectations.txt

@@ -381,4 +381,4 @@ crbug.com/1151797 [ win10 amd-0x7340 skia-renderer-gl ] Pixel_DirectComposition_
 crbug.com/1151797 [ win10 amd-0x7340 skia-renderer-gl ] Pixel_DirectComposition_Video_VP9_BGRA [ Failure ]
 
 # Flakes on Mac dual-GPU
-crbug.com/1069339 [ mojave amd-0x6821 ] Pixel_OffscreenCanvasIBRCWebGLHighPerfWorker [ RetryOnFailure ]
+crbug.com/1069339 [ mojave amd-0x6821 ] Pixel_OffscreenCanvasIBRCWebGLHighPerfWorker [ RetryOnFailure ]  # finder:disable Very low flake rate (1 in 1k+ builds)

content/test/gpu/unexpected_pass_finder.py

@@ -19,6 +19,14 @@ Concrete example:
 unexpected_pass_finder.py \
   --project luci-resultdb-dev \
   --suite pixel
+
+You would typically want to pass in --remove-stale-expectations as well in order
+to have the script automatically remove any expectations it determines are no
+longer necessary. If a particular expectation proves to be erroneously flagged
+and removed (e.g. due to a very low flake rate that doesn't get caught
+consistently by the script), expectations can be omitted from automatic removal
+using an inline `# finder:disable` comment for a single expectation or a pair of
+`# finder:disable`/`# finder:enable` comments for a block of expectations.
 """
 
 import argparse

content/test/gpu/unexpected_passes/expectations.py

@@ -11,6 +11,10 @@ from typ import expectations_parser
 from unexpected_passes import data_types
 
 
+FINDER_DISABLE_COMMENT = 'finder:disable'
+FINDER_ENABLE_COMMENT = 'finder:enable'
+
+
 def CreateTestExpectationMap(expectation_file, tests):
   """Creates an expectation map based off a file or list of tests.
 
@@ -190,6 +194,9 @@ def SplitExpectationsByStaleness(test_expectation_map):
 def RemoveExpectationsFromFile(expectations, expectation_file):
   """Removes lines corresponding to |expectations| from |expectation_file|.
 
+  Ignores any lines that match but are within a disable block or have an inline
+  disable comment.
+
   Args:
     expectations: A list of data_types.Expectations to remove.
     expectation_file: A filepath pointing to an expectation file to remove lines
@@ -201,11 +208,31 @@ def RemoveExpectationsFromFile(expectations, expectation_file):
     input_contents = f.read()
 
   output_contents = ''
+  in_disable_block = False
+  disable_block_reason = ''
   for line in input_contents.splitlines(True):
     # Auto-add any comments or empty lines
     stripped_line = line.strip()
     if not stripped_line or stripped_line.startswith('#'):
       output_contents += line
+      assert not (FINDER_DISABLE_COMMENT in line
+                  and FINDER_ENABLE_COMMENT in line)
+      # Handle disable/enable block comments.
+      if FINDER_DISABLE_COMMENT in line:
+        if in_disable_block:
+          raise RuntimeError(
+              'Invalid expectation file %s - contains a disable comment "%s" '
+              'that is in another disable block.' %
+              (expectation_file, stripped_line))
+        in_disable_block = True
+        disable_block_reason = _GetDisableReasonFromComment(line)
+      if FINDER_ENABLE_COMMENT in line:
+        if not in_disable_block:
+          raise RuntimeError(
+              'Invalid expectation file %s - contains an enable comment "%s" '
+              'that is outside of a disable block.' %
+              (expectation_file, stripped_line))
+        in_disable_block = False
       continue
 
     single_line_content = header + line
@@ -219,8 +246,26 @@ def RemoveExpectationsFromFile(expectations, expectation_file):
 
     # Add any lines containing expectations that don't match any of the given
     # expectations to remove.
-    if not any([e for e in expectations if e == current_expectation]):
+    if any([e for e in expectations if e == current_expectation]):
+      # Skip any expectations that match if we're in a disable block or there
+      # is an inline disable comment.
+      if in_disable_block:
+        output_contents += line
+        logging.info(
+            'Would have removed expectation %s, but inside a disable block '
+            'with reason %s', stripped_line, disable_block_reason)
+      elif FINDER_DISABLE_COMMENT in line:
+        output_contents += line
+        logging.info(
+            'Would have removed expectation %s, but it has an inline disable '
+            'comment with reason %s',
+            stripped_line.split('#')[0], _GetDisableReasonFromComment(line))
+    else:
       output_contents += line
 
   with open(expectation_file, 'w') as f:
     f.write(output_contents)
+
+
+def _GetDisableReasonFromComment(line):
+  return line.split(FINDER_DISABLE_COMMENT, 1)[1].strip()

content/test/gpu/unexpected_passes/expectations_unittest.py

@@ -369,6 +369,83 @@ crbug.com/2345 [ win ] foo/test [ RetryOnFailure ]
     with open(self.filename) as f:
       self.assertEqual(f.read(), expected_contents)
 
+  def testNestedBlockComments(self):
+    """Tests that nested disable block comments throw exceptions."""
+    contents = validate_tag_consistency.TAG_HEADER + """
+# finder:disable
+# finder:disable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+# finder:enable
+# finder:enable
+"""
+    with open(self.filename, 'w') as f:
+      f.write(contents)
+    with self.assertRaises(RuntimeError):
+      expectations.RemoveExpectationsFromFile([], self.filename)
+
+    contents = validate_tag_consistency.TAG_HEADER + """
+# finder:enable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+"""
+    with open(self.filename, 'w') as f:
+      f.write(contents)
+    with self.assertRaises(RuntimeError):
+      expectations.RemoveExpectationsFromFile([], self.filename)
+
+  def testBlockComments(self):
+    """Tests that expectations in a disable block comment are not removed."""
+    contents = validate_tag_consistency.TAG_HEADER + """
+crbug.com/1234 [ win ] foo/test [ Failure ]
+# finder:disable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+crbug.com/1234 [ win ] foo/test [ Failure ]
+# finder:enable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+"""
+    stale_expectations = [
+        data_types.Expectation('foo/test', ['win'], ['Failure'])
+    ]
+    expected_contents = validate_tag_consistency.TAG_HEADER + """
+# finder:disable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+crbug.com/1234 [ win ] foo/test [ Failure ]
+# finder:enable
+"""
+    with open(self.filename, 'w') as f:
+      f.write(contents)
+    expectations.RemoveExpectationsFromFile(stale_expectations, self.filename)
+    with open(self.filename) as f:
+      self.assertEqual(f.read(), expected_contents)
+
+  def testInlineComments(self):
+    """Tests that expectations with inline disable comments are not removed."""
+    contents = validate_tag_consistency.TAG_HEADER + """
+crbug.com/1234 [ win ] foo/test [ Failure ]
+crbug.com/1234 [ win ] foo/test [ Failure ]  # finder:disable
+crbug.com/1234 [ win ] foo/test [ Failure ]
+"""
+    stale_expectations = [
+        data_types.Expectation('foo/test', ['win'], ['Failure'])
+    ]
+    expected_contents = validate_tag_consistency.TAG_HEADER + """
+crbug.com/1234 [ win ] foo/test [ Failure ]  # finder:disable
+"""
+    with open(self.filename, 'w') as f:
+      f.write(contents)
+    expectations.RemoveExpectationsFromFile(stale_expectations, self.filename)
+    with open(self.filename) as f:
+      self.assertEqual(f.read(), expected_contents)
+
+  def testGetDisableReasonFromComment(self):
+    """Tests that the disable reason can be pulled from a line."""
+    self.assertEqual(
+        expectations._GetDisableReasonFromComment('# finder:disable foo'),
+        'foo')
+    self.assertEqual(
+        expectations._GetDisableReasonFromComment(
+            'crbug.com/1234 [ win ] bar/test [ Failure ]  # finder:disable foo'
+        ), 'foo')
+
 
 if __name__ == '__main__':
   unittest.main(verbosity=2)

content/test/gpu/unexpected_passes/queries.py

@@ -49,7 +49,7 @@ WITH
         AND STRUCT("builder", @builder_name) IN UNNEST(variant)
         AND REGEXP_CONTAINS(
           test_id,
-          r"gpu_tests\.{suite}_integration_test\.")
+          r"gpu_tests\.{suite}\.")
       GROUP BY exported.id
       ORDER BY ANY_VALUE(partition_time) DESC
       LIMIT @num_builds
@@ -66,6 +66,15 @@ SELECT tr.*
 FROM tests t, t.test_results tr
 """
 
+# The suite reported to Telemetry for selecting which suite to run is not
+# necessarily the same one that is reported to typ/ResultDB, so map any special
+# cases here.
+TELEMETRY_SUITE_TO_RDB_SUITE_EXCEPTION_MAP = {
+    'info_collection': 'info_collection_test',
+    'power': 'power_measurement_integration_test',
+    'trace_test': 'trace_integration_test',
+}
+
 
 def FillExpectationMapForCiBuilders(expectation_map, builders, suite, project,
                                     num_samples):
@@ -243,6 +252,12 @@ def QueryBuilder(builder, builder_type, suite, project, num_samples):
   else:
     check_webgl_version = lambda tags: True
 
+  # Most test names are |suite|_integration_test, but there are several that
+  # are not reported that way in typ, and by extension ResultDB, so adjust that
+  # here.
+  suite = TELEMETRY_SUITE_TO_RDB_SUITE_EXCEPTION_MAP.get(
+      suite, suite + '_integration_test')
+
   query = GPU_BQ_QUERY_TEMPLATE.format(builder_type=builder_type, suite=suite)
   cmd = [
       'bq',

content/test/gpu/unexpected_passes/queries_unittest.py

@@ -444,6 +444,54 @@ class QueryBuilderUnittest(unittest.TestCase):
         data_types.Result('test_name', ['webgl-version-2'], 'Failure',
                           'step_name', '2345'))
 
+  def testSuiteExceptionMap(self):
+    """Tests that the suite passed to the query changes for some suites."""
+    # These don't actually matter, we just need to ensure that something valid
+    # is returned so QueryBuilder doesn't explode.
+    query_results = [
+        {
+            'id':
+            'build-1234',
+            'test_id': ('ninja://chrome/test:telemetry_gpu_integration_test/'
+                        'gpu_tests.webgl_conformance_integration_test.'
+                        'WebGLConformanceIntegrationTest.test_name'),
+            'status':
+            'FAIL',
+            'typ_expectations': [
+                'RetryOnFailure',
+            ],
+            'typ_tags': [
+                'webgl-version-1',
+            ],
+            'step_name':
+            'step_name',
+        },
+    ]
+    self._process_mock.return_value = json.dumps(query_results)
+
+    def assertSuiteInQuery(suite, call_args):
+      cmd = call_args[0][0]
+      s = 'r"gpu_tests\\.%s\\."' % suite
+      for c in cmd:
+        if s in c:
+          return
+      self.fail()
+
+    # Non-special cased suite.
+    _, _ = queries.QueryBuilder('builder', 'ci', 'pixel', 'project', 5)
+    assertSuiteInQuery('pixel_integration_test', self._process_mock.call_args)
+
+    # Special-cased suites.
+    _, _ = queries.QueryBuilder('builder', 'ci', 'info_collection', 'project',
+                                5)
+    assertSuiteInQuery('info_collection_test', self._process_mock.call_args)
+    _, _ = queries.QueryBuilder('builder', 'ci', 'power', 'project', 5)
+    assertSuiteInQuery('power_measurement_integration_test',
+                       self._process_mock.call_args)
+
+    _, _ = queries.QueryBuilder('builder', 'ci', 'trace_test', 'project', 5)
+    assertSuiteInQuery('trace_integration_test', self._process_mock.call_args)
+
 
 class FillExpectationMapForBuildersUnittest(unittest.TestCase):
   def setUp(self):