Add arg replacement to generate_buildbot_json.py

Adds the ability to replace or remove arguments for tests generated
using generate_buildbot_json.py by specifying a 'replacements' field in
test_suite_exceptions.pyl.

This is useful in cases where a subset of bots have slightly different
args from usual. Previously, this required a duplicate test suite
definition with the updated args. With this change, the same definition
can be used with the differences listed in the exceptions.

Bug: 980631
Change-Id: If3c0c22f2046ce67123ec06dea88a6bf8de9102f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1691266Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676270}

testing/buildbot/README.md

@@ -269,11 +269,23 @@ The exceptions file supports the following options per test:
   bot. This can be used to add additional command line arguments, Swarming
   parameters, etc.
 
-* `key_removals`: a dictionary mapping a bot's name to a list of keys which
-  should be removed from the test's specification on that bot. Note that by
-  design, this feature *can not* be used to remove Swarming dictionary entries.
-  This feature was mainly used to match the previously handwritten JSON files,
-  should not be used in the future, and should ideally be removed.
+* `replacements`: a dictionary mapping bot names to a dictionaries of field
+  names to dictionaries of key/value pairs to replace. If the given value is
+  `None`, then the key will simply be removed. For example:
+  ```
+  'foo_tests': {
+    'Foo Tester': {
+      'args': {
+        '--some-flag': None,
+        '--another-flag': 'some-value',
+      },
+    },
+  }
+  ```
+  would remove the `--some-flag` and replace whatever value `--another-flag` was
+  set to with `some-value`. Note that passing `None` only works if the flag
+  being removed either has no value or is in the `--key=value` format. It does
+  not work if the key and value are two separate entries in the args list.
 
 ### Order of application of test changes
 

testing/buildbot/generate_buildbot_json.py

@@ -279,6 +279,12 @@ class BBJSONGenerator(object):
       return None
     return exception.get('modifications', {}).get(tester_name)
 
+  def get_test_replacements(self, test, test_name, tester_name):
+    exception = self.get_exception_for_test(test_name, test)
+    if not exception:
+      return None
+    return exception.get('replacements', {}).get(tester_name)
+
   def merge_command_line_args(self, arr, prefix, splitter):
     prefix_len = len(prefix)
     idx = 0
@@ -447,9 +453,44 @@ class BBJSONGenerator(object):
       for d in test['swarming'].get('dimension_sets', []):
         if d.get('os') == 'Android' and not d.get('device_os_type'):
           d['device_os_type'] = 'userdebug'
+    self.replace_test_args(test, test_name, tester_name)
 
     return test
 
+  def replace_test_args(self, test, test_name, tester_name):
+    replacements = self.get_test_replacements(
+        test, test_name, tester_name) or {}
+    valid_replacement_keys = ['args', 'non_precommit_args', 'precommit_args']
+    for key, replacement_dict in replacements.iteritems():
+      if key not in valid_replacement_keys:
+        raise BBGenErr(
+            'Given replacement key %s for %s on %s is not in the list of valid '
+            'keys %s' % (key, test_name, tester_name, valid_replacement_keys))
+      for replacement_key, replacement_val in replacement_dict.iteritems():
+        found_key = False
+        for i, test_key in enumerate(test.get(key, [])):
+          # Handle both the key/value being replaced being defined as two
+          # separate items or as key=value.
+          if test_key == replacement_key:
+            found_key = True
+            # Handle flags without values.
+            if replacement_val == None:
+              del test[key][i]
+            else:
+              test[key][i+1] = replacement_val
+            break
+          elif test_key.startswith(replacement_key + '='):
+            found_key = True
+            if replacement_val == None:
+              del test[key][i]
+            else:
+              test[key][i] = '%s=%s' % (replacement_key, replacement_val)
+            break
+        if not found_key:
+          raise BBGenErr('Could not find %s in existing list of values for key '
+                         '%s in %s on %s' % (replacement_key, key, test_name,
+                             tester_name))
+
   def add_common_test_properties(self, test, tester_config):
     if tester_config.get('use_multi_dimension_trigger_script'):
       # Assumes update_and_cleanup_test has already been called, so the

testing/buildbot/generate_buildbot_json_unittest.py

@@ -3852,5 +3852,231 @@ class QueryTests(unittest.TestCase):
       self.assertEqual(cm.exception.code, 1)
     self.assertTrue(fbb.printed_lines)
 
+
+FOO_TEST_SUITE_WITH_ENABLE_FEATURES_SEPARATE_ENTRIES = """\
+{
+  'basic_suites': {
+    'foo_tests': {
+      'foo_test': {
+        'args': [
+          '--enable-features',
+          'Foo,Bar',
+        ],
+      },
+    },
+  },
+}
+"""
+
+
+FOO_TEST_REPLACEMENTS_REMOVE_NO_VALUE = """\
+{
+  'foo_test': {
+    'replacements': {
+      'Fake Tester': {
+        'args': {
+          '--c_arg': None,
+        },
+      },
+    },
+  },
+}
+"""
+
+
+FOO_TEST_REPLACEMENTS_REMOVE_VALUE = """\
+{
+  'foo_test': {
+    'replacements': {
+      'Fake Tester': {
+        'args': {
+          '--enable-features': None,
+        },
+      },
+    },
+  },
+}
+"""
+
+
+FOO_TEST_REPLACEMENTS_REPLACE_VALUE = """\
+{
+  'foo_test': {
+    'replacements': {
+      'Fake Tester': {
+        'args': {
+          '--enable-features': 'Bar,Baz',
+        },
+      },
+    },
+  },
+}
+"""
+
+
+FOO_TEST_REPLACEMENTS_INVALID_KEY = """\
+{
+  'foo_test': {
+    'replacements': {
+      'Fake Tester': {
+        'invalid': {
+          '--enable-features': 'Bar,Baz',
+        },
+      },
+    },
+  },
+}
+"""
+
+
+REPLACEMENTS_REMOVE_OUTPUT = """\
+{
+  "AAAAA1 AUTOGENERATED FILE DO NOT EDIT": {},
+  "AAAAA2 See generate_buildbot_json.py to make changes": {},
+  "Fake Tester": {
+    "gtest_tests": [
+      {
+        "args": [],
+        "merge": {
+          "args": [],
+          "script": "//testing/merge_scripts/standard_gtest_merge.py"
+        },
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "foo_test"
+      }
+    ]
+  }
+}
+"""
+
+REPLACEMENTS_VALUE_OUTPUT = """\
+{
+  "AAAAA1 AUTOGENERATED FILE DO NOT EDIT": {},
+  "AAAAA2 See generate_buildbot_json.py to make changes": {},
+  "Fake Tester": {
+    "gtest_tests": [
+      {
+        "args": [
+          "--enable-features=Bar,Baz"
+        ],
+        "merge": {
+          "args": [],
+          "script": "//testing/merge_scripts/standard_gtest_merge.py"
+        },
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "foo_test"
+      }
+    ]
+  }
+}
+"""
+
+REPLACEMENTS_VALUE_SEPARATE_ENTRIES_OUTPUT = """\
+{
+  "AAAAA1 AUTOGENERATED FILE DO NOT EDIT": {},
+  "AAAAA2 See generate_buildbot_json.py to make changes": {},
+  "Fake Tester": {
+    "gtest_tests": [
+      {
+        "args": [
+          "--enable-features",
+          "Bar,Baz"
+        ],
+        "merge": {
+          "args": [],
+          "script": "//testing/merge_scripts/standard_gtest_merge.py"
+        },
+        "swarming": {
+          "can_use_on_swarming_builders": true,
+          "dimension_sets": [
+            {
+              "kvm": "1"
+            }
+          ]
+        },
+        "test": "foo_test"
+      }
+    ]
+  }
+}
+"""
+
+
+class ReplacementTests(unittest.TestCase):
+  """Tests for the arg replacement feature."""
+  def test_replacement_valid_remove_no_value(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ARGS,
+                    FOO_TEST_REPLACEMENTS_REMOVE_NO_VALUE,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    fbb.files['chromium.test.json'] = REPLACEMENTS_REMOVE_OUTPUT
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_replacement_valid_remove_value(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ENABLE_FEATURES,
+                    FOO_TEST_REPLACEMENTS_REMOVE_VALUE,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    fbb.files['chromium.test.json'] = REPLACEMENTS_REMOVE_OUTPUT
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_replacement_valid_replace_value(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ENABLE_FEATURES,
+                    FOO_TEST_REPLACEMENTS_REPLACE_VALUE,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    fbb.files['chromium.test.json'] = REPLACEMENTS_VALUE_OUTPUT
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_replacement_valid_replace_value_separate_entries(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ENABLE_FEATURES_SEPARATE_ENTRIES,
+                    FOO_TEST_REPLACEMENTS_REPLACE_VALUE,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    fbb.files['chromium.test.json'] = REPLACEMENTS_VALUE_SEPARATE_ENTRIES_OUTPUT
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_replacement_invalid_key_not_valid(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE,
+                    FOO_TEST_REPLACEMENTS_INVALID_KEY,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    with self.assertRaisesRegexp(generate_buildbot_json.BBGenErr,
+        'Given replacement key *'):
+      fbb.check_output_file_consistency(verbose=True)
+
+  def test_replacement_invalid_key_not_found(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ARGS,
+                    FOO_TEST_REPLACEMENTS_REPLACE_VALUE,
+                    EMPTY_PYL_FILE,
+                    LUCI_MILO_CFG)
+    with self.assertRaisesRegexp(generate_buildbot_json.BBGenErr,
+        'Could not find *'):
+      fbb.check_output_file_consistency(verbose=True)
+
 if __name__ == '__main__':
   unittest.main()

testing/buildbot/test_suite_exceptions.pyl

@@ -1231,7 +1231,39 @@
       },
     },
   },
- 'pixel_test':{
+  'pixel_skia_gold_test': {
+    'replacements': {
+      # client.v8.fyi
+      # The V8 builders pass the V8 revision for ${got_revision}, so instead
+      # use ${got_cr_revision}, which is only set on the V8 bots.
+      'Android V8 FYI Release (Nexus 5X)': {
+        'args': {
+          '--build-revision': '${got_cr_revision}',
+        },
+      },
+      'Linux V8 FYI Release (NVIDIA)': {
+        'args': {
+          '--build-revision': '${got_cr_revision}',
+        },
+      },
+      'Linux V8 FYI Release - pointer compression (NVIDIA)': {
+        'args': {
+          '--build-revision': '${got_cr_revision}',
+        },
+      },
+      'Mac V8 FYI Release (Intel)': {
+        'args': {
+          '--build-revision': '${got_cr_revision}',
+        },
+      },
+      'Win V8 FYI Release (NVIDIA)': {
+        'args': {
+          '--build-revision': '${got_cr_revision}',
+        },
+      },
+    },
+  },
+  'pixel_test':{
     'modifications': {
       'Android Release (Nexus 5X)': {
         'swarming': {

testing/buildbot/test_suites.pyl

@@ -3713,49 +3713,6 @@
       },
     },
 
-    # This is identical to 'gpu_telemetry_tests_for_skia_gold_testing', except
-    # that the --build-revision arg uses ${got_cr_revision} instead of
-    # ${got_revision}. This is because got_revision on V8 builders contains
-    # the V8 revision, not the Chromium revision, which confuses the GPU team's
-    # Skia Gold instance since it's tracking the Chromium src repo.
-    'gpu_telemetry_tests_for_skia_gold_testing_v8': {
-      'pixel_skia': {
-        'name': 'pixel_skia_gold_test',
-        'args': [
-          '--dont-restore-color-profile-after-test',
-          '--refimg-cloud-storage-bucket',
-          'chromium-gpu-archive/reference-images',
-          '--os-type',
-          '${os_type}',
-          '--build-revision',
-          '${got_cr_revision}',
-          '--test-machine-name',
-          '${buildername}',
-          '--use-skia-gold',
-        ],
-        'non_precommit_args': [
-          '--upload-refimg-to-cloud-storage',
-        ],
-        'precommit_args': [
-          '--download-refimg-from-cloud-storage',
-          # Gerrit issue ID
-          '--review-patch-issue',
-          '${patch_issue}',
-          # Patch set number
-          '--review-patch-set',
-          '${patch_set}',
-          # Buildbucket ID
-          '--buildbucket-build-id',
-          '${buildbucket_build_id}',
-        ],
-        'experiment_percentage': 100,
-        'swarming': {
-          'service_account': 'chrome-gpu-gold@chops-service-accounts.iam.gserviceaccount.com'
-        },
-        'telemetry_test_name': 'pixel',
-      },
-    },
-
     'gpu_webgl2_conformance_d3d11_validating_telemetry_tests': {
       'webgl2_conformance_d3d11_validating_tests': {
         'args': [
@@ -5422,7 +5379,7 @@
     'gpu_nexus5x_telemetry_tests_v8': [
       'gpu_common_and_optional_telemetry_tests',
       'gpu_telemetry_tests',
-      'gpu_telemetry_tests_for_skia_gold_testing_v8',
+      'gpu_telemetry_tests_for_skia_gold_testing',
       'gpu_webgl_conformance_gles_passthrough_telemetry_tests',
       'gpu_webgl_conformance_telemetry_tests',
     ],
@@ -5453,7 +5410,7 @@
     'gpu_v8_desktop_telemetry_tests': [
       'gpu_common_and_optional_telemetry_tests',
       'gpu_telemetry_tests',
-      'gpu_telemetry_tests_for_skia_gold_testing_v8',
+      'gpu_telemetry_tests_for_skia_gold_testing',
       'gpu_webgl2_conformance_telemetry_tests',
       'gpu_webgl_conformance_telemetry_tests',
     ],