Handle test suite exceptions the same as mixins.
Currently generate_buildbot_json.py uses self.dictionary_merge to apply
changes in the modifications in test_suite_exceptions.pyl whereas mixins
use per-key logic for a handful of keys and then just uses the dict
update method to overwrite the entries for the remaining keys. The
dictionary_merge method recursively applies to dictionaries and when
merging two dicts, if the dict being merged in has None for a value,
that entry will be removed from the dict being merged into.
The different implementations result in swarming dimensions being
handled differently between a mixin and a test suite modification: a
dimension with a None value will be kept (written out as a json null) if
it was part of a mixin but the dimension will be removed if a test suite
exception sets a dimension to None. Since per test modifications in
starlark are handled via mixins, this creates an incompatibility for the
migration.
There are some instances of the null-value dimensions actually being
required since the recipe provides a default cpu dimension of x86-64 for
OSes besides CrOS and Android, so a null cpu dimension is required for
Fuchsia to remove the recipe-provided default. Additionally, the
resultdb field isn't merged despite it being a dict, so dictionary_merge
is ill-suited for handling the entirety of the mixins. To align the
behavior in both cases, the test suite exceptions will be applied as
though they were mixins, which aligns well with the starlark behavior.
This additionally removes the dictionary_merge method and replaces it
with a merg_swarming method to ensure that the other places that were
merging swarming dicts use the same behavior.
Bug: 40258588
Change-Id: I39b559c00dbdb2a09a6afa1aa5d6b35c6ae21890
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5902494
Commit-Queue: Garrett Beaty <[email protected]>
Reviewed-by: Stephanie Kim <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1362708}
diff --git a/testing/buildbot/generate_buildbot_json.py b/testing/buildbot/generate_buildbot_json.py
index feeb0d1..62a5d3d 100755
--- a/testing/buildbot/generate_buildbot_json.py
+++ b/testing/buildbot/generate_buildbot_json.py
@@ -545,33 +545,15 @@
if substituted_array != original_args:
test_config['args'] = self.maybe_fixup_args_array(substituted_array)
- def dictionary_merge(self, a, b, path=None):
- """http://stackoverflow.com/questions/7204805/
- python-dictionaries-of-dictionaries-merge
- merges b into a
- """
- if path is None:
- path = []
- for key in b:
- 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):
- a[key] = a[key] + b[key]
- if key.endswith('args'):
- a[key] = self.maybe_fixup_args_array(a[key])
- elif b[key] is None:
- del a[key]
- else:
- a[key] = b[key]
-
- return a
+ @staticmethod
+ def merge_swarming(swarming1, swarming2):
+ swarming2 = dict(swarming2)
+ if 'dimensions' in swarming2:
+ swarming1.setdefault('dimensions', {}).update(swarming2.pop('dimensions'))
+ if 'named_caches' in swarming2:
+ named_caches = swarming1.setdefault('named_caches', [])
+ named_caches.extend(swarming2.pop('named_caches'))
+ swarming1.update(swarming2)
def clean_swarming_dictionary(self, swarming_dict):
# Clean out redundant entries from a test's "swarming" dictionary.
@@ -593,7 +575,7 @@
):
swarming = test.pop(key, None)
if swarming and fn(builder):
- self.dictionary_merge(test['swarming'], swarming)
+ self.merge_swarming(test['swarming'], swarming)
for key, fn in (
('desktop_args', lambda cfg: not self.is_android(cfg)),
@@ -659,7 +641,7 @@
test = self.apply_mixins(test, variant_mixins, mixins_to_ignore, builder)
# Add any swarming or args from the builder
- self.dictionary_merge(test['swarming'], builder.get('swarming', {}))
+ self.merge_swarming(test['swarming'], builder.get('swarming', {}))
if supports_args:
test.setdefault('args', []).extend(builder.get('args', []))
@@ -679,7 +661,7 @@
# test's specification.
modifications = self.get_test_modifications(test, builder_name)
if modifications:
- test = self.dictionary_merge(test, modifications)
+ test = self.apply_mixin(modifications, test, builder)
# Clean up the swarming entry or remove it if it's unnecessary
if (swarming_dict := test.get('swarming')) is not None:
@@ -1407,20 +1389,8 @@
new_test['description'] = '\n'.join(description)
if 'swarming' in mixin:
- swarming_mixin = mixin['swarming']
- new_test.setdefault('swarming', {})
- if 'dimensions' in swarming_mixin:
- new_test['swarming'].setdefault('dimensions', {}).update(
- swarming_mixin.pop('dimensions'))
- if 'named_caches' in swarming_mixin:
- new_test['swarming'].setdefault('named_caches', []).extend(
- swarming_mixin['named_caches'])
- del swarming_mixin['named_caches']
- # python dict update doesn't do recursion at all. Just hard code the
- # nested update we need (mixin['swarming'] shouldn't clobber
- # test['swarming'], but should update it).
- new_test['swarming'].update(swarming_mixin)
- del mixin['swarming']
+ self.merge_swarming(new_test.setdefault('swarming', {}),
+ mixin.pop('swarming'))
for a in ('args', 'precommit_args', 'non_precommit_args'):
if (value := mixin.pop(a, None)) is None:
