buildbot gen: Better sorting

This CL modifies the sorting and duplicate checks in the *.pyl
files in //testing/buildbot. It adds more checks for subsections
of the files; for example, each entry inside of test_suite_exceptions.pyl
will now be checked, rather than just checking that the top level
list of tests is sorted.

A few duplicates have been removed; these won't cause any meaningful
behavior change, since python ignores duplicates in dictionaries.

Change-Id: I314338c904080b463ac6859011a538987a862530
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1986241
Commit-Queue: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Garrett Beaty <gbeaty@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728678}

testing/buildbot/generate_buildbot_json.py

@@ -1299,44 +1299,99 @@ class BBJSONGenerator(object):
               filename, node_dumped,
               node.lineno, typ, type(node)))
 
-  def ensure_ast_dict_keys_sorted(self, node, filename, verbose):
-    is_valid = True
+  def check_ast_list_formatted(self, keys, filename, verbose,
+                               check_sorting=False):
+    """Checks if a list of ast keys are correctly formatted.
 
+    Currently only checks to ensure they're correctly sorted, and that there
+    are no duplicates.
+
+    Args:
+      keys: An python list of AST nodes.
+
+            It's a list of AST nodes instead of a list of strings because
+            when verbose is set, it tries to print out context of where the
+            diffs are in the file.
+      filename: The name of the file this node is from.
+      verbose: If set, print out diff information about how the keys are
+               incorrectly formatted.
+      check_sorting: If true, checks if the list is sorted.
+    Returns:
+      If the keys are correctly formatted.
+    """
+    if not keys:
+      return True
+
+    assert isinstance(keys[0], ast.Str)
+
+    keys_strs = [k.s for k in keys]
+    # Keys to diff against. Used below.
+    keys_to_diff_against = None
+    # If the list is properly formatted.
+    list_formatted = True
+
+    # Duplicates are always bad.
+    if len(set(keys_strs)) != len(keys_strs):
+      list_formatted = False
+      keys_to_diff_against = list(collections.OrderedDict.fromkeys(keys_strs))
+
+    if check_sorting and sorted(keys_strs) != keys_strs:
+      list_formatted = False
+    if list_formatted:
+      return True
+
+    if verbose:
+      line_num = keys[0].lineno
+      keys = [k.s for k in keys]
+      if check_sorting:
+        # If we have duplicates, sorting this will take care of it anyways.
+        keys_to_diff_against = sorted(set(keys))
+      # else, keys_to_diff_against is set above already
+
+      self.print_line('=' * 80)
+      self.print_line('(First line of keys is %s)' % line_num)
+      for line in difflib.context_diff(
+          keys, keys_to_diff_against,
+          fromfile='current (%r)' % filename, tofile='sorted', lineterm=''):
+        self.print_line(line)
+      self.print_line('=' * 80)
+
+    return False
+
+  def check_ast_dict_formatted(self, node, filename, verbose,
+                               check_sorting=True):
+    """Checks if an ast dictionary's keys are correctly formatted.
+
+    Just a simple wrapper around check_ast_list_formatted.
+    Args:
+      node: An AST node. Assumed to be a dictionary.
+      filename: The name of the file this node is from.
+      verbose: If set, print out diff information about how the keys are
+               incorrectly formatted.
+      check_sorting: If true, checks if the list is sorted.
+    Returns:
+      If the dictionary is correctly formatted.
+    """
     keys = []
     # The keys of this dict are ordered as ordered in the file; normal python
     # dictionary keys are given an arbitrary order, but since we parsed the
     # file itself, the order as given in the file is preserved.
     for key in node.keys:
       self.type_assert(key, ast.Str, filename, verbose)
-      keys.append(key.s)
+      keys.append(key)
 
-    keys_sorted = sorted(keys)
-    if keys_sorted != keys:
-      is_valid = False
-      if verbose:
-        for line in difflib.unified_diff(
-            keys,
-            keys_sorted, fromfile='current (%r)' % filename, tofile='sorted'):
-          self.print_line(line)
-
-    if len(set(keys)) != len(keys):
-      for i in range(len(keys_sorted)-1):
-        if keys_sorted[i] == keys_sorted[i+1]:
-          self.print_line('Key %s is duplicated' % keys_sorted[i])
-          is_valid = False
-    return is_valid
+    return self.check_ast_list_formatted(
+        keys, filename, verbose, check_sorting=check_sorting)
 
   def check_input_files_sorting(self, verbose=False):
     # TODO(https://crbug.com/886993): Add the ability for this script to
     # actually format the files, rather than just complain if they're
     # incorrectly formatted.
     bad_files = set()
+    def parse_file(filename):
+      """Parses and validates a .pyl file.
 
-    for filename in (
-        'mixins.pyl',
-        'test_suites.pyl',
-        'test_suite_exceptions.pyl',
-    ):
+      Returns an AST node representing the value in the pyl file."""
       parsed = ast.parse(self.read_file(self.pyl_file_path(filename)))
 
       # Must be a module.
@@ -1350,10 +1405,47 @@ class BBJSONGenerator(object):
       expr = module[0]
       self.type_assert(expr, ast.Expr, filename, verbose)
 
+      return expr.value
+
+    # Handle this separately
+    filename = 'waterfalls.pyl'
+    value = parse_file(filename)
+    # Value should be a list.
+    self.type_assert(value, ast.List, filename, verbose)
+
+    keys = []
+    for val in value.elts:
+      self.type_assert(val, ast.Dict, filename, verbose)
+      waterfall_name = None
+      for key, val in zip(val.keys, val.values):
+        self.type_assert(key, ast.Str, filename, verbose)
+        if key.s == 'machines':
+          if not self.check_ast_dict_formatted(val, filename, verbose):
+            bad_files.add(filename)
+
+        if key.s == "name":
+          self.type_assert(val, ast.Str, filename, verbose)
+          waterfall_name = val
+      assert waterfall_name
+      keys.append(waterfall_name)
+
+    if not self.check_ast_list_formatted(
+        keys, filename, verbose, check_sorting=True):
+      bad_files.add(filename)
+
+    for filename in (
+        'mixins.pyl',
+        'test_suites.pyl',
+        'test_suite_exceptions.pyl',
+    ):
+      value = parse_file(filename)
       # Value should be a dictionary.
-      value = expr.value
       self.type_assert(value, ast.Dict, filename, verbose)
 
+      if not self.check_ast_dict_formatted(
+          value, filename, verbose):
+        bad_files.add(filename)
+
       if filename == 'test_suites.pyl':
         expected_keys = ['basic_suites',
                          'compound_suites',
@@ -1366,62 +1458,41 @@ class BBJSONGenerator(object):
         # Only two keys should mean only 1 or 2 values
         assert len(suite_dicts) <= 3
         for suite_group in suite_dicts:
-          if not self.ensure_ast_dict_keys_sorted(
+          if not self.check_ast_dict_formatted(
               suite_group, filename, verbose):
             bad_files.add(filename)
 
-      else:
-        if not self.ensure_ast_dict_keys_sorted(
-            value, filename, verbose):
+        for key, suite in zip(value.keys, value.values):
+          # The compound suites are checked in
+          # 'check_composition_type_test_suites()'
+          if key.s == 'basic_suites':
+            for group in suite.values:
+              if not self.check_ast_dict_formatted(
+                  group, filename, verbose, check_sorting=False):
                 bad_files.add(filename)
+            break
 
-    # waterfalls.pyl is slightly different, just do it manually here
-    filename = 'waterfalls.pyl'
-    parsed = ast.parse(self.read_file(self.pyl_file_path(filename)))
-
-    # Must be a module.
-    self.type_assert(parsed, ast.Module, filename, verbose)
-    module = parsed.body
-
-    # Only one expression in the module.
-    self.type_assert(module, list, filename, verbose)
-    if len(module) != 1: # pragma: no cover
-      raise BBGenErr('Invalid .pyl file %s' % filename)
-    expr = module[0]
-    self.type_assert(expr, ast.Expr, filename, verbose)
-
-    # Value should be a list.
-    value = expr.value
-    self.type_assert(value, ast.List, filename, verbose)
-
-    keys = []
-    for val in value.elts:
-      self.type_assert(val, ast.Dict, filename, verbose)
-      waterfall_name = None
-      for key, val in zip(val.keys, val.values):
-        self.type_assert(key, ast.Str, filename, verbose)
-        if key.s == 'machines':
-          if not self.ensure_ast_dict_keys_sorted(val, filename, verbose):
+      elif filename == 'test_suite_exceptions.pyl':
+        # Check the values for each test.
+        for test in value.values:
+          for kind, node in zip(test.keys, test.values):
+            if isinstance(node, ast.Dict):
+              if not self.check_ast_dict_formatted(
+                  node, filename, verbose, check_sorting=False):
                 bad_files.add(filename)
-
-        if key.s == "name":
-          self.type_assert(val, ast.Str, filename, verbose)
-          waterfall_name = val.s
-      assert waterfall_name
-      keys.append(waterfall_name)
-
-    if sorted(keys) != keys:
+            elif kind.s == 'remove_from':
+              # Don't care about sorting; these are usually grouped, since the
+              # same bug can affect multiple builders. Do want to make sure
+              # there aren't duplicates.
+              if not self.check_ast_list_formatted(node.elts, filename, verbose,
+                                               check_sorting=False):
                 bad_files.add(filename)
-      if verbose: # pragma: no cover
-        for line in difflib.unified_diff(
-            keys,
-            sorted(keys), fromfile='current', tofile='sorted'):
-          self.print_line(line)
 
     if bad_files:
       raise BBGenErr(
           'The following files have invalid keys: %s\n. They are either '
-          'unsorted, or have duplicates.' % ', '.join(bad_files))
+          'unsorted, or have duplicates. Re-run this with --verbose to see '
+          'more details.' % ', '.join(bad_files))
 
   def check_output_file_consistency(self, verbose=False):
     self.load_configuration_files()

testing/buildbot/generate_buildbot_json_unittest.py

@@ -573,6 +573,17 @@ FOO_TEST_SUITE = """\
 }
 """
 
+FOO_TEST_SUITE_NOT_SORTED = """\
+{
+  'basic_suites': {
+    'foo_tests': {
+      'foo_test': {},
+      'a_test': {},
+    },
+  },
+}
+"""
+
 FOO_TEST_SUITE_WITH_ARGS = """\
 {
   'basic_suites': {
@@ -913,6 +924,37 @@ EXCEPTIONS_UNSORTED = """\
 }
 """
 
+EXCEPTIONS_PER_TEST_UNSORTED = """\
+{
+  'suite_d': {
+    'modifications': {
+      'Other Tester': {
+        'foo': 'baz',
+      },
+      'Fake Tester': {
+        'foo': 'baz',
+      },
+    },
+  },
+}
+"""
+
+EXCEPTIONS_DUPS_REMOVE_FROM = """\
+{
+  'suite_d': {
+    'remove_from': [
+      'Fake Tester',
+      'Fake Tester',
+    ],
+    'modifications': {
+      'Fake Tester': {
+        'foo': 'baz',
+      },
+    },
+  },
+}
+"""
+
 FOO_TEST_MODIFICATIONS = """\
 {
   'foo_test': {
@@ -2454,9 +2496,9 @@ class UnitTest(unittest.TestCase):
       fbb.check_input_file_consistency(verbose=True)
     joined_lines = '\n'.join(fbb.printed_lines)
     self.assertRegexpMatches(
-      joined_lines, '.*\+chromium\..*test.*')
+      joined_lines, '.*\+ chromium\..*test.*')
     self.assertRegexpMatches(
-      joined_lines, '.*\-chromium\..*test.*')
+      joined_lines, '.*\- chromium\..*test.*')
     fbb.printed_lines = []
     self.assertFalse(fbb.printed_lines)
 
@@ -2483,6 +2525,57 @@ class UnitTest(unittest.TestCase):
     fbb.check_input_file_consistency(verbose=True)
     self.assertFalse(fbb.printed_lines)
 
+    fbb = FakeBBGen(TEST_SUITE_SORTING_WATERFALL,
+                    TEST_SUITE_SORTED,
+                    LUCI_MILO_CFG,
+                    exceptions=EXCEPTIONS_DUPS_REMOVE_FROM)
+    with self.assertRaises(generate_buildbot_json.BBGenErr):
+      fbb.check_input_file_consistency(verbose=True)
+    joined_lines = ' '.join(fbb.printed_lines)
+    self.assertRegexpMatches(
+        joined_lines, '.*\- Fake Tester.*')
+    fbb.printed_lines = []
+    self.assertFalse(fbb.printed_lines)
+
+  def test_test_suite_exceptions_no_dups_remove_from(self):
+    fbb = FakeBBGen(TEST_SUITE_SORTING_WATERFALL,
+                    TEST_SUITE_SORTED,
+                    LUCI_MILO_CFG,
+                    exceptions=EXCEPTIONS_SORTED)
+    fbb.check_input_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+    fbb = FakeBBGen(TEST_SUITE_SORTING_WATERFALL,
+                    TEST_SUITE_SORTED,
+                    LUCI_MILO_CFG,
+                    exceptions=EXCEPTIONS_PER_TEST_UNSORTED)
+    real_dict_format = fbb.check_ast_dict_formatted
+    def fake_dict_format(*args, **kwargs):
+      kwargs['check_sorting'] = True
+      return real_dict_format(*args, **kwargs)
+
+    # Mock this out for now. A future CL will enable sorting by default. Still
+    # want to test the codepath, even if it's not used it production yet.
+    fbb.check_ast_dict_formatted = fake_dict_format
+
+    with self.assertRaises(generate_buildbot_json.BBGenErr):
+      fbb.check_input_file_consistency(verbose=True)
+    joined_lines = ' '.join(fbb.printed_lines)
+    self.assertRegexpMatches(
+        joined_lines, '.*\+ Fake Tester.*')
+    self.assertRegexpMatches(
+        joined_lines, '.*\- Fake Tester.*')
+    fbb.printed_lines = []
+    self.assertFalse(fbb.printed_lines)
+
+  def test_test_suite_exceptions_per_test_must_be_sorted(self):
+    fbb = FakeBBGen(TEST_SUITE_SORTING_WATERFALL,
+                    TEST_SUITE_SORTED,
+                    LUCI_MILO_CFG,
+                    exceptions=EXCEPTIONS_SORTED)
+    fbb.check_input_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
     fbb = FakeBBGen(TEST_SUITE_SORTING_WATERFALL,
                     TEST_SUITE_SORTED,
                     LUCI_MILO_CFG,
@@ -2491,9 +2584,9 @@ class UnitTest(unittest.TestCase):
       fbb.check_input_file_consistency(verbose=True)
     joined_lines = ' '.join(fbb.printed_lines)
     self.assertRegexpMatches(
-        joined_lines, '.*\+suite_.*')
+        joined_lines, '.*\+ suite_.*')
     self.assertRegexpMatches(
-        joined_lines, '.*\-suite_.*')
+        joined_lines, '.*\- suite_.*')
     fbb.printed_lines = []
     self.assertFalse(fbb.printed_lines)
 
@@ -2516,9 +2609,9 @@ class UnitTest(unittest.TestCase):
         fbb.check_input_file_consistency(verbose=True)
       joined_lines = ' '.join(fbb.printed_lines)
       self.assertRegexpMatches(
-          joined_lines, '.*\+suite_.*')
+          joined_lines, '.*\+ suite_.*')
       self.assertRegexpMatches(
-          joined_lines, '.*\-suite_.*')
+          joined_lines, '.*\- suite_.*')
       fbb.printed_lines = []
       self.assertFalse(fbb.printed_lines)
 
@@ -3031,11 +3124,11 @@ class MixinTests(unittest.TestCase):
                     mixins=SWARMING_MIXINS_UNSORTED)
     with self.assertRaises(generate_buildbot_json.BBGenErr):
       fbb.check_input_file_consistency(verbose=True)
-    joined_lines = ' '.join(fbb.printed_lines)
+    joined_lines = '\n'.join(fbb.printed_lines)
     self.assertRegexpMatches(
-        joined_lines, '.*\+._mixin.*')
+        joined_lines, '.*\+ ._mixin.*')
     self.assertRegexpMatches(
-        joined_lines, '.*\-._mixin.*')
+        joined_lines, '.*\- ._mixin.*')
     fbb.printed_lines = []
     self.assertFalse(fbb.printed_lines)
 
@@ -3153,9 +3246,32 @@ class MixinTests(unittest.TestCase):
         generate_buildbot_json.BBGenErr,
         'The following files have invalid keys: mixins.pyl'):
       fbb.check_input_file_consistency(verbose=True)
-    joined_lines = ' '.join(fbb.printed_lines)
+    joined_lines = '\n'.join(fbb.printed_lines)
     self.assertRegexpMatches(
-        joined_lines, 'Key .* is duplicated')
+        joined_lines, '.*\- builder_mixin')
+    fbb.printed_lines = []
+    self.assertFalse(fbb.printed_lines)
+
+  def test_no_duplicate_keys_basic_test_suite(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_NOT_SORTED,
+                    LUCI_MILO_CFG)
+    real_dict_format = fbb.check_ast_dict_formatted
+    def fake_dict_format(*args, **kwargs):
+      kwargs['check_sorting'] = True
+      return real_dict_format(*args, **kwargs)
+
+    # Mock this out for now. A future CL will enable sorting by default. Still
+    # want to test the codepath, even if it's not used it production yet.
+    fbb.check_ast_dict_formatted = fake_dict_format
+
+    with self.assertRaisesRegexp(
+        generate_buildbot_json.BBGenErr,
+        'The following files have invalid keys: test_suites.pyl'):
+      fbb.check_input_file_consistency(verbose=True)
+    joined_lines = '\n'.join(fbb.printed_lines)
+    self.assertRegexpMatches(joined_lines, '.*\- a_test')
+    self.assertRegexpMatches(joined_lines, '.*\+ a_test')
     fbb.printed_lines = []
     self.assertFalse(fbb.printed_lines)
 

testing/buildbot/test_suite_exceptions.pyl

@@ -715,22 +715,6 @@
         },
         'use_xvfb': False,
       },
-      'Linux ASan LSan Tests (1)': {
-        'args': [
-          '--use-gpu-in-tests',
-          '--no-xvfb',
-        ],
-        'swarming': {
-          'dimension_sets': [
-            {
-              'gpu': '10de:1cb3',
-              'os': 'Ubuntu',
-              'pool': 'chromium.tests.gpu',
-            },
-          ],
-        },
-        'use_xvfb': False,
-      },
       'Linux CFI': {
         'args': [
           '--use-gpu-in-tests',
@@ -1133,7 +1117,6 @@
       'Mac10.13 Tests',
       'Mac10.13 Tests (dbg)',
       'Mac ASan 64 Tests (1)',
-      'Mac ASan 64 Tests (1)',
       'ToTMacASan',
     ],
     'modifications': {
@@ -1165,7 +1148,6 @@
       'Mac10.13 Tests',
       'Mac10.13 Tests (dbg)',
       'Mac ASan 64 Tests (1)',
-      'Mac ASan 64 Tests (1)',
       'ToTMacASan',
     ],
   },
@@ -1186,7 +1168,6 @@
       'Mac10.13 Tests',
       'Mac10.13 Tests (dbg)',
       'Mac ASan 64 Tests (1)',
-      'Mac ASan 64 Tests (1)',
       'ToTMacASan',
     ],
   },
@@ -1206,7 +1187,6 @@
       'Mac10.13 Tests',
       'Mac10.13 Tests (dbg)',
       'Mac ASan 64 Tests (1)',
-      'Mac ASan 64 Tests (1)',
       'ToTMacASan',
     ],
   },
@@ -1232,7 +1212,6 @@
       'Mac10.13 Tests',
       'Mac10.13 Tests (dbg)',
       'Mac ASan 64 Tests (1)',
-      'Mac ASan 64 Tests (1)',
       'ToTMacASan',
     ],
     'modifications': {

testing/buildbot/test_suites.pyl

@@ -3276,12 +3276,6 @@
                  '--test-launcher-timeout=120000'],
         'test': 'content_browsertests',
       },
-      'content_browsertests_stress': {
-        'args': ['--gtest_filter=WebRtc*MANUAL*:-UsingRealWebcam*',
-                 '--run-manual', '--ui-test-action-max-timeout=110000',
-                 '--test-launcher-timeout=120000'],
-        'test': 'content_browsertests',
-      },
       'browser_tests_functional': {
         'args': [
           '--test-launcher-filter-file=../../testing/buildbot/filters/webrtc_functional.browser_tests.filter',