Add buildbot JSON magic substitutions

Adds the ability to use magic substitution args when generating buildbot
JSON files, i.e. magic values that will be expanded to different
arguments depending on the test config.

Also switches the CrOS Telemetry tests to use this to set different
arguments for tests running on VMs and physical hardware instead of
using test_suite_exceptions.pyl.

This is a prerequisite for expanding GPU test coverage on CrOS, as the
differing arguments between VMs and hardware will be difficult to
maintain using test_suite_exceptions.pyl given the large number of GPU
Telemetry tests. There may be other use cases for this feature, though.

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

testing/buildbot/PRESUBMIT.py

@@ -27,6 +27,13 @@ def CommonChecks(input_api, output_api):
         input_api.python_executable, 'generate_buildbot_json_coveragetest.py'],
       kwargs={}, message=output_api.PresubmitError),
 
+    input_api.Command(
+      name='buildbot_json_magic_substitutions_unittest', cmd=[
+        input_api.python_executable,
+        'buildbot_json_magic_substitutions_unittest.py',
+      ], kwargs={}, message=output_api.PresubmitError
+    ),
+
     input_api.Command(
       name='manage', cmd=[
         input_api.python_executable, 'manage.py', '--check'],

testing/buildbot/buildbot_json_magic_substitutions.py

@@ -0,0 +1,66 @@
+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""A set of functions to programmatically substitute test arguments.
+
+Arguments for a test that start with $$MAGIC_SUBSTITUTION_ will be replaced with
+the output of the corresponding function in this file. For example,
+$$MAGIC_SUBSTITUTION_Foo would be replaced with the return value of the Foo()
+function.
+
+This is meant as an alternative to many entries in test_suite_exceptions.pyl if
+the differentiation can be done programmatically.
+"""
+
+MAGIC_SUBSTITUTION_PREFIX = '$$MAGIC_SUBSTITUTION_'
+
+
+def ChromeOSTelemetryRemote(test_config):
+  """Substitutes the correct CrOS remote Telemetry arguments.
+
+  VMs use a hard-coded remote address and port, while physical hardware use
+  a magic hostname.
+
+  Args:
+    test_config: A dict containing a configuration for a specific test on a
+        specific builder.
+  """
+  def StringContainsSubstring(s, sub_strs):
+    for sub_str in sub_strs:
+      if sub_str in s:
+        return True
+    return False
+  VM_POOLS = [
+    'chromium.tests.cros.vm',
+    'chrome.tests.cros-vm',
+  ]
+  HW_POOLS = [
+    'chrome-cros-dut',
+    'chrome.cros-dut',
+  ]
+  dimensions = test_config.get('swarming', {}).get('dimension_sets', [])
+  assert len(dimensions)
+  pool = dimensions[0].get('pool')
+  if not pool:
+    raise RuntimeError(
+        'No pool set for CrOS test, unable to determine whether running on '
+        'a VM or physical hardware.')
+
+  if StringContainsSubstring(pool, VM_POOLS):
+    return [
+      '--remote=127.0.0.1',
+      # By default, CrOS VMs' ssh servers listen on local port 9222.
+      '--remote-ssh-port=9222',
+    ]
+  if StringContainsSubstring(pool, HW_POOLS):
+    return [
+      # Magic hostname that resolves to a CrOS device in the test lab.
+      '--remote=variable_chromeos_device_hostname',
+    ]
+  raise RuntimeError('Unknown CrOS pool %s' % pool)
+
+
+def TestOnlySubstitution(_):
+  """Magic substitution used for unittests."""
+  return ['--magic-substitution-success']

testing/buildbot/buildbot_json_magic_substitutions_unittest.py

@@ -0,0 +1,50 @@
+#!/usr/bin/python
+# Copyright 2020 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import unittest
+
+import buildbot_json_magic_substitutions as magic_substitutions
+
+
+def CreateConfigWithPool(pool):
+  return {
+    'swarming': {
+      'dimension_sets': [
+        {
+          'pool': pool,
+        },
+      ],
+    },
+  }
+
+
+class ChromeOSTelemetryRemoteTest(unittest.TestCase):
+
+  def testVirtualMachineSubstitutions(self):
+    test_config = CreateConfigWithPool('chromium.tests.cros.vm')
+    self.assertEqual(magic_substitutions.ChromeOSTelemetryRemote(test_config),
+                     [
+                       '--remote=127.0.0.1',
+                       '--remote-ssh-port=9222',
+                     ])
+
+  def testPhysicalHardwareSubstitutions(self):
+    test_config = CreateConfigWithPool('chrome-cros-dut')
+    self.assertEqual(magic_substitutions.ChromeOSTelemetryRemote(test_config),
+                     ['--remote=variable_chromeos_device_hostname'])
+
+  def testNoPool(self):
+    test_config = CreateConfigWithPool(None)
+    with self.assertRaisesRegexp(RuntimeError, 'No pool *'):
+      magic_substitutions.ChromeOSTelemetryRemote(test_config)
+
+  def testUnknownPool(self):
+    test_config = CreateConfigWithPool('totally-legit-pool')
+    with self.assertRaisesRegexp(RuntimeError, 'Unknown CrOS pool *'):
+      magic_substitutions.ChromeOSTelemetryRemote(test_config)
+
+
+if __name__ == '__main__':
+  unittest.main()

testing/buildbot/generate_buildbot_json.py

@@ -21,6 +21,8 @@
 import sys
 import traceback
 
+import buildbot_json_magic_substitutions as magic_substitutions
+
 THIS_DIR = os.path.dirname(os.path.abspath(__file__))
 
 
@@ -413,6 +415,34 @@ def maybe_fixup_args_array(self, arr):
     arr = self.merge_command_line_args(arr, '--extra-browser-args=', ' ')
     return arr
 
+  def substitute_magic_args(self, test_config):
+    """Substitutes any magic substitution args present in |test_config|.
+
+    Substitutions are done in-place.
+
+    See buildbot_json_magic_substitutions.py for more information on this
+    feature.
+
+    Args:
+      test_config: A dict containing a configuration for a specific test on
+          a specific builder, e.g. the output of update_and_cleanup_test.
+    """
+    substituted_array = []
+    for arg in test_config.get('args', []):
+      if arg.startswith(magic_substitutions.MAGIC_SUBSTITUTION_PREFIX):
+        function = arg.replace(
+            magic_substitutions.MAGIC_SUBSTITUTION_PREFIX, '')
+        if hasattr(magic_substitutions, function):
+          substituted_array.extend(
+              getattr(magic_substitutions, function)(test_config))
+        else:
+          raise BBGenErr(
+              'Magic substitution function %s does not exist' % function)
+      else:
+        substituted_array.append(arg)
+    if substituted_array:
+      test_config['args'] = self.maybe_fixup_args_array(substituted_array)
+
   def dictionary_merge(self, a, b, path=None, update=True):
     """http://stackoverflow.com/questions/7204805/
         python-dictionaries-of-dictionaries-merge
@@ -670,6 +700,7 @@ def generate_gtest(self, waterfall, tester_name, tester_config, test_name,
     result = self.update_and_cleanup_test(
         result, test_name, tester_name, tester_config, waterfall)
     self.add_common_test_properties(result, tester_config)
+    self.substitute_magic_args(result)
 
     if not result.get('merge'):
       # TODO(https://crbug.com/958376): Consider adding the ability to not have
@@ -695,6 +726,7 @@ def generate_isolated_script_test(self, waterfall, tester_name, tester_config,
     result = self.update_and_cleanup_test(
         result, test_name, tester_name, tester_config, waterfall)
     self.add_common_test_properties(result, tester_config)
+    self.substitute_magic_args(result)
 
     if not result.get('merge'):
       # TODO(https://crbug.com/958376): Consider adding the ability to not have
@@ -722,6 +754,7 @@ def generate_script_test(self, waterfall, tester_name, tester_config,
     }
     result = self.update_and_cleanup_test(
         result, test_name, tester_name, tester_config, waterfall)
+    self.substitute_magic_args(result)
     return result
 
   def generate_junit_test(self, waterfall, tester_name, tester_config,
@@ -737,6 +770,7 @@ def generate_junit_test(self, waterfall, tester_name, tester_config,
     self.initialize_args_for_test(result, tester_config)
     result = self.update_and_cleanup_test(
         result, test_name, tester_name, tester_config, waterfall)
+    self.substitute_magic_args(result)
     return result
 
   def generate_instrumentation_test(self, waterfall, tester_name, tester_config,
@@ -751,6 +785,7 @@ def generate_instrumentation_test(self, waterfall, tester_name, tester_config,
       result['test'] = test_name
     result = self.update_and_cleanup_test(
         result, test_name, tester_name, tester_config, waterfall)
+    self.substitute_magic_args(result)
     return result
 
   def substitute_gpu_args(self, tester_config, swarming_config, args):

testing/buildbot/generate_buildbot_json_unittest.py

@@ -4834,6 +4834,86 @@ def test_replacement_invalid_key_not_found(self):
       fbb.check_output_file_consistency(verbose=True)
 
 
+FOO_TEST_SUITE_WITH_MAGIC_ARGS = """\
+{
+  'basic_suites': {
+    'foo_tests': {
+      'foo_test': {
+        'args': [
+          '$$MAGIC_SUBSTITUTION_TestOnlySubstitution',
+        ],
+      },
+    },
+  },
+}
+"""
+
+
+FOO_TEST_SUITE_WITH_INVALID_MAGIC_ARGS = """\
+{
+  'basic_suites': {
+    'foo_tests': {
+      'foo_test': {
+        'args': [
+          '$$MAGIC_SUBSTITUTION_NotARealSubstitution',
+        ],
+      },
+    },
+  },
+}
+"""
+
+
+MAGIC_SUBSTITUTIONS_OUTPUT = """\
+{
+  "AAAAA1 AUTOGENERATED FILE DO NOT EDIT": {},
+  "AAAAA2 See generate_buildbot_json.py to make changes": {},
+  "Fake Tester": {
+    "gtest_tests": [
+      {
+        "args": [
+          "--magic-substitution-success"
+        ],
+        "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 MagicSubstitutionTests(unittest.TestCase):
+  """Tests for the magic substitution feature."""
+  def test_valid_function(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_MAGIC_ARGS,
+                    LUCI_MILO_CFG)
+    fbb.files['chromium.test.json'] = MAGIC_SUBSTITUTIONS_OUTPUT
+    fbb.files['chromium.ci.json'] = MAGIC_SUBSTITUTIONS_OUTPUT
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_invalid_function(self):
+    fbb = FakeBBGen(FOO_GTESTS_WATERFALL,
+                    FOO_TEST_SUITE_WITH_INVALID_MAGIC_ARGS,
+                    LUCI_MILO_CFG)
+    with self.assertRaisesRegexp(generate_buildbot_json.BBGenErr,
+        'Magic substitution function *'):
+      fbb.check_output_file_consistency(verbose=True)
+
+
 # Matrix compound composition test suites
 
 MATRIX_COMPOUND_EMPTY = """\

testing/buildbot/test_suite_exceptions.pyl

@@ -2413,16 +2413,6 @@
         },
       },
     },
-    'replacements': {
-      'chromeos-kevin-rel-hw-tests': {
-        # Replace VM args with the magic hostname that resolves to a CrOS
-        # device in the test lab.
-        'args': {
-          '--remote': 'variable_chromeos_device_hostname',
-          '--remote-ssh-port': None,
-        },
-      },
-    },
     'remove_from': [
       # Too slow on this configuration, which is severely hardware
       # constrained. crbug.com/950690

testing/buildbot/test_suites.pyl

@@ -471,9 +471,7 @@
       'telemetry_perf_unittests': {
         'args': [
           '--browser=cros-chrome',
-          # By default, CrOS VMs' ssh servers listen on local port 9222.
-          '--remote=127.0.0.1',
-          '--remote-ssh-port=9222',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
           '--xvfb',
         ],
         'swarming': {
@@ -485,9 +483,7 @@
         'args': [
           '--jobs=1',
           '--browser=cros-chrome',
-          # By default, CrOS VMs' ssh servers listen on local port 9222.
-          '--remote=127.0.0.1',
-          '--remote-ssh-port=9222',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
         ],
         'swarming': {
           'idempotent': False,  # https://crbug.com/549140
@@ -500,8 +496,7 @@
       'telemetry_perf_unittests': {
         'args': [
           '--browser=cros-chrome',
-          # The magic hostname that resolves to a CrOS device in the test lab.
-          '--remote=variable_chromeos_device_hostname',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
           '--xvfb',
         ],
         'swarming': {
@@ -512,8 +507,7 @@
       'telemetry_unittests': {
         'args': [
           '--browser=cros-chrome',
-          # The magic hostname that resolves to a CrOS device in the test lab.
-          '--remote=variable_chromeos_device_hostname',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
           '--jobs=1',
         ],
         'swarming': {
@@ -2600,12 +2594,7 @@
     'gpu_webgl_conformance_telemetry_tests': {
       'webgl_conformance': {
         'chromeos_args': [
-          # If we're testing on CrOS, assume we're running on VMs. So point
-          # telemetry to the default SSH port of the VM. Note that this won't
-          # work for hardware tests.
-          # TODO(bpastene): Make this work for hardware tests.
-          '--remote=127.0.0.1',
-          '--remote-ssh-port=9222',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
         ],
         'args': [
           # On dual-GPU devices we want the high-performance GPU to be active
@@ -2628,12 +2617,7 @@
       'webgl_conformance_validating': {
         'telemetry_test_name': 'webgl_conformance',
         'chromeos_args': [
-          # If we're testing on CrOS, assume we're running on VMs. So point
-          # telemetry to the default SSH port of the VM. Note that this won't
-          # work for hardware tests.
-          # TODO(bpastene): Make this work for hardware tests.
-          '--remote=127.0.0.1',
-          '--remote-ssh-port=9222',
+          '$$MAGIC_SUBSTITUTION_ChromeOSTelemetryRemote',
         ],
         'args': [
           # On dual-GPU devices we want the high-performance GPU to be active