[infra] More closely unify variant and mixin behavior.
Variants are logically similar to mixins in their ability to modify a
test suite, with the difference being that a test spec is created for
each variant rather than all being applied to a single test spec. To
ensure more consistent treatment of fields, variants will now use the
mixin codepath to modify a test.
The following differences remain with variants:
* The enabled, identifier and mixins fields specify behavior that is
specific to variants.
* Skylab details can be specified on a variant under the skylab key.
When a variant is applied to a test, if the skylab key is present, all
fields of the value will be lifted to the top-level, except for the
cros_chrome_version field, which is only used by the cros skylab image
roller to compare against other data sources.
This CL introduces the following behavior changes:
* The swarming field in a variant is merged in the same way as with a
mixin, in particular, dimension_sets will be added to the test's
dimension_sets value instead of merging elements at the same index.
This didn't result in any changes to generated files, but all variants
have been updated to use dimensions instead of dimension_sets to
ensure the same behavior going forward.
* A mixin that specifies the description field (non exist at this
point), will have the description appended to any existing description
for the test.
Bug: 1456553
Change-Id: I869d8baad76027d811b7c3b96ed35d46509a819b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4703284
Auto-Submit: Garrett Beaty <[email protected]>
Commit-Queue: Ben Pastene <[email protected]>
Reviewed-by: Ben Pastene <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1173053}
diff --git a/testing/buildbot/generate_buildbot_json.py b/testing/buildbot/generate_buildbot_json.py
index c59087a9..7c31d02 100755
--- a/testing/buildbot/generate_buildbot_json.py
+++ b/testing/buildbot/generate_buildbot_json.py
@@ -605,7 +605,7 @@
if substituted_array != original_args:
test_config['args'] = self.maybe_fixup_args_array(substituted_array)
- def dictionary_merge(self, a, b, path=None, update=True):
+ def dictionary_merge(self, a, b, path=None):
"""http://stackoverflow.com/questions/7204805/
python-dictionaries-of-dictionaries-merge
merges b into a
@@ -613,44 +613,42 @@
if path is None:
path = []
for key in b:
- if key in a:
- if isinstance(a[key], dict) and isinstance(b[key], dict):
- self.dictionary_merge(a[key], b[key], path + [str(key)])
- elif a[key] == b[key]:
- pass # same leaf value
- elif isinstance(a[key], list) and isinstance(b[key], list):
- # Args arrays are lists of strings. Just concatenate them,
- # and don't sort them, in order to keep some needed
- # arguments adjacent (like --timeout-ms [arg], etc.)
- if all(isinstance(x, str)
- for x in itertools.chain(a[key], b[key])):
- a[key] = self.maybe_fixup_args_array(a[key] + b[key])
- else:
- # TODO(kbr): this only works properly if the two arrays are
- # the same length, which is currently always the case in the
- # swarming dimension_sets that we have to merge. It will fail
- # to merge / override 'args' arrays which are different
- # length.
- for idx in range(len(b[key])):
- try:
- a[key][idx] = self.dictionary_merge(a[key][idx], b[key][idx],
- path + [str(key), str(idx)],
- update=update)
- except (IndexError, TypeError) as e:
- raise BBGenErr('Error merging lists by key "%s" from source %s '
- 'into target %s at index %s. Verify target list '
- 'length is equal or greater than source' %
- (str(key), str(b), str(a), str(idx))) from e
- elif update:
- if b[key] is None:
- del a[key]
- else:
- a[key] = b[key]
+ if key not in a:
+ if b[key] is not None:
+ a[key] = b[key]
+ continue
+
+ if isinstance(a[key], dict) and isinstance(b[key], dict):
+ self.dictionary_merge(a[key], b[key], path + [str(key)])
+ elif a[key] == b[key]:
+ pass # same leaf value
+ elif isinstance(a[key], list) and isinstance(b[key], list):
+ # Args arrays are lists of strings. Just concatenate them,
+ # and don't sort them, in order to keep some needed
+ # arguments adjacent (like --timeout-ms [arg], etc.)
+ if all(isinstance(x, str) for x in itertools.chain(a[key], b[key])):
+ a[key] = self.maybe_fixup_args_array(a[key] + b[key])
else:
- raise BBGenErr('Conflict at %s' % '.'.join(
- path + [str(key)])) # pragma: no cover
- elif b[key] is not None:
+ # TODO(kbr): this only works properly if the two arrays are
+ # the same length, which is currently always the case in the
+ # swarming dimension_sets that we have to merge. It will fail
+ # to merge / override 'args' arrays which are different
+ # length.
+ for idx in range(len(b[key])):
+ try:
+ a[key][idx] = self.dictionary_merge(a[key][idx], b[key][idx],
+ path +
+ [str(key), str(idx)])
+ except (IndexError, TypeError) as e: # pragma: no cover
+ raise BBGenErr('Error merging lists by key "%s" from source %s '
+ 'into target %s at index %s. Verify target list '
+ 'length is equal or greater than source' %
+ (str(key), str(b), str(a), str(idx))) from e
+ elif b[key] is None:
+ del a[key]
+ else:
a[key] = b[key]
+
return a
def initialize_args_for_test(
@@ -1211,80 +1209,51 @@
# Each test in a basic test suite will have a definition per variant.
test_suite = {}
- for test_name, test_config in basic_test_definition.items():
- definitions = []
- for variant in variants:
- # Unpack the variant from variants.pyl if it's string based.
- if isinstance(variant, str):
- variant = self.variants[variant]
+ for variant in variants:
+ # Unpack the variant from variants.pyl if it's string based.
+ if isinstance(variant, str):
+ variant = self.variants[variant]
- # If 'enabled' is set to False, we will not use this variant;
- # otherwise if the variant doesn't include 'enabled' variable or
- # 'enabled' is set to True, we will use this variant
- if not variant.get('enabled', True):
- continue
- # Clone a copy of test_config so that we can have a uniquely updated
- # version of it per variant
- cloned_config = copy.deepcopy(test_config)
- # The variant definition needs to be re-used for each test, so we'll
- # create a clone and work with it as well.
- cloned_variant = copy.deepcopy(variant)
+ # If 'enabled' is set to False, we will not use this variant; otherwise if
+ # the variant doesn't include 'enabled' variable or 'enabled' is set to
+ # True, we will use this variant
+ if not variant.get('enabled', True):
+ continue
- cloned_config['args'] = (cloned_config.get('args', []) +
- cloned_variant.get('args', []))
- cloned_config['mixins'] = (cloned_config.get('mixins', []) +
- cloned_variant.get('mixins', []) + mixins)
+ # Make a shallow copy of the variant to remove variant-specific fields,
+ # leaving just mixin fields
+ variant = copy.copy(variant)
+ variant.pop('enabled', None)
+ identifier = variant.pop('identifier')
+ variant_mixins = variant.pop('mixins', [])
+ variant_skylab = variant.pop('skylab', {})
- description = []
- if cloned_config.get('description'):
- description.append(cloned_config.get('description'))
- if cloned_variant.get('description'):
- description.append(cloned_variant.get('description'))
- if description:
- cloned_config['description'] = '\n'.join(description)
- basic_swarming_def = cloned_config.get('swarming', {})
- variant_swarming_def = cloned_variant.get('swarming', {})
- if basic_swarming_def and variant_swarming_def:
- if ('dimension_sets' in basic_swarming_def and
- 'dimension_sets' in variant_swarming_def):
- # Retain swarming dimension set merge behavior when both variant and
- # the basic test configuration both define it
- self.dictionary_merge(basic_swarming_def, variant_swarming_def)
- # Remove dimension_sets from the variant definition, so that it does
- # not replace what's been done by dictionary_merge in the update
- # call below.
- del variant_swarming_def['dimension_sets']
+ for test_name, test_config in basic_test_definition.items():
+ new_test = self.apply_mixin(variant, test_config)
- # Update the swarming definition with whatever is defined for swarming
- # by the variant.
- basic_swarming_def.update(variant_swarming_def)
- cloned_config['swarming'] = basic_swarming_def
-
- # Copy all skylab fields defined by the variant.
- skylab_config = cloned_variant.get('skylab')
- if skylab_config:
- for k, v in skylab_config.items():
- # cros_chrome_version is the ash chrome version in the cros img
- # in the variant of cros_board. We don't want to include it in
- # the final json files; so remove it.
- if k == 'cros_chrome_version':
- continue
- cloned_config[k] = v
+ new_test['mixins'] = (test_config.get('mixins', []) + variant_mixins +
+ mixins)
# The identifier is used to make the name of the test unique.
# Generators in the recipe uniquely identify a test by it's name, so we
# don't want to have the same name for each variant.
- cloned_config['name'] = self.add_variant_to_test_name(
- cloned_config.get('name') or test_name,
- cloned_variant['identifier'])
+ new_test['name'] = self.add_variant_to_test_name(
+ new_test.get('name', test_name), identifier)
# Attach the variant identifier to the test config so downstream
# generators can make modifications based on the original name. This
# is mainly used in generate_gpu_telemetry_test().
- cloned_config['variant_id'] = cloned_variant['identifier']
+ new_test['variant_id'] = identifier
- definitions.append(cloned_config)
- test_suite[test_name] = definitions
+ # cros_chrome_version is the ash chrome version in the cros img in the
+ # variant of cros_board. We don't want to include it in the final json
+ # files; so remove it.
+ for k, v in variant_skylab.items():
+ if k != 'cros_chrome_version':
+ new_test[k] = v
+
+ test_suite.setdefault(test_name, []).append(new_test)
+
return test_suite
def resolve_matrix_compound_test_suites(self):
@@ -1424,7 +1393,7 @@
del test['mixins']
return test
- def apply_mixin(self, mixin, test, builder):
+ def apply_mixin(self, mixin, test, builder=None):
"""Applies a mixin to a test.
A mixin is applied by copying all fields from the mixin into the
@@ -1445,6 +1414,14 @@
new_test = copy.deepcopy(test)
mixin = copy.deepcopy(mixin)
+
+ if 'description' in mixin:
+ description = []
+ if 'description' in new_test:
+ description.append(new_test['description'])
+ description.append(mixin.pop('description'))
+ new_test['description'] = '\n'.join(description)
+
if 'swarming' in mixin:
swarming_mixin = mixin['swarming']
new_test.setdefault('swarming', {})
@@ -1501,6 +1478,8 @@
args = new_test.get('args', [])
def add_conditional_args(key, fn):
+ if builder is None:
+ return
val = new_test.pop(key, [])
if val and fn(builder):
args.extend(val)
