[M116][infra] Remove the need to use $mixin_append.

The $mixin_append field is used for appending rather than overwriting
any of the various *args fields or the named_caches field of the
swarming field, but there are no uses of the *args fields that are not
in $mixin_append and none of the non-$mixin_append occurrences of
named_caches is overwriting any other value. This simplifies the
behavior so that named_caches and *args fields will always be appended
rather than requiring the $mixin_append field. Once all mixins.pyl files
are updated, the $mixin_append handling can be removed.

(cherry picked from commit 4c35b14)

Bug: 1456553
Change-Id: Ic29508b2b86ac47f3f70472de6e97aadfe6beaf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4632065
Commit-Queue: Garrett Beaty <gbeaty@google.com>
Reviewed-by: Struan Shrimpton <sshrimp@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1161935}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4651928
Auto-Submit: Garrett Beaty <gbeaty@google.com>
Commit-Queue: Struan Shrimpton <sshrimp@google.com>
Cr-Commit-Position: refs/branch-heads/5845@{chromium#145}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

testing/buildbot/generate_buildbot_json.py

@@ -1426,15 +1426,22 @@ def must_be_list(mixins, typ, name):
   def apply_mixin(self, mixin, test, builder):
     """Applies a mixin to a test.
 
-    Mixins will not override an existing key. This is to ensure exceptions can
-    override a setting a mixin applies.
-
-    Swarming dimensions are handled in a special way. Instead of specifying
-    'dimension_sets', which is how normal test suites specify their dimensions,
-    you specify a 'dimensions' key, which maps to a dictionary. This dictionary
-    is then applied to every dimension set in the test.
-
+    A mixin is applied by copying all fields from the mixin into the
+    test with the following exceptions:
+    * For the various *args keys, the test's existing value (an empty
+      list if not present) will be extended with the mixin's value.
+    * The sub-keys of the swarming value will be copied to the test's
+      swarming value with the following exceptions:
+      * For the dimension_sets and named_caches sub-keys, the test's
+        existing value (an empty list if not present) will be extended
+        with the mixin's value.
+      * For the dimensions sub-key, after extending the test's
+        dimension_sets as specified above, each dimension set will be
+        updated with the value of the dimensions sub-key. If there are
+        no dimension sets, then one will be added that contains the
+        specified dimensions.
     """
+
     new_test = copy.deepcopy(test)
     mixin = copy.deepcopy(mixin)
     if 'swarming' in mixin:
@@ -1458,12 +1465,39 @@ def apply_mixin(self, mixin, test, builder):
         for dimension_set in new_test['swarming']['dimension_sets']:
           dimension_set.update(swarming_mixin['dimensions'])
         del swarming_mixin['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']
 
+    # Array so we can assign to it in a nested scope.
+    args_need_fixup = ['args' in mixin]
+
+    for a in (
+        'args',
+        'precommit_args',
+        'non_precommit_args',
+        'desktop_args',
+        'lacros_args',
+        'linux_args',
+        'android_args',
+        'chromeos_args',
+        'mac_args',
+        'win_args',
+        'win64_args',
+    ):
+      if (value := mixin.pop(a, None)) is None:
+        continue
+      if not isinstance(value, list):
+        raise BBGenErr(f'"{a}" must be a list')
+      new_test.setdefault(a, []).extend(value)
+
+    # TODO(gbeaty) Remove this once all mixins have removed '$mixin_append'
     if '$mixin_append' in mixin:
       # Values specified under $mixin_append should be appended to existing
       # lists, rather than replacing them.
@@ -1491,29 +1525,28 @@ def apply_mixin(self, mixin, test, builder):
               'Cannot apply $mixin_append to non-list "' + key + '".')
         new_test[key].extend(mixin_append[key])
 
-      args = new_test.get('args', [])
-      # Array so we can assign to it in a nested scope.
-      args_need_fixup = [False]
       if 'args' in mixin_append:
         args_need_fixup[0] = True
 
-      def add_conditional_args(key, fn):
-        val = new_test.pop(key, [])
-        if val and fn(builder):
-          args.extend(val)
-          args_need_fixup[0] = True
-
-      add_conditional_args('desktop_args', lambda cfg: not self.is_android(cfg))
-      add_conditional_args('lacros_args', self.is_lacros)
-      add_conditional_args('linux_args', self.is_linux)
-      add_conditional_args('android_args', self.is_android)
-      add_conditional_args('chromeos_args', self.is_chromeos)
-      add_conditional_args('mac_args', self.is_mac)
-      add_conditional_args('win_args', self.is_win)
-      add_conditional_args('win64_args', self.is_win64)
-
-      if args_need_fixup[0]:
-        new_test['args'] = self.maybe_fixup_args_array(args)
+    args = new_test.get('args', [])
+
+    def add_conditional_args(key, fn):
+      val = new_test.pop(key, [])
+      if val and fn(builder):
+        args.extend(val)
+        args_need_fixup[0] = True
+
+    add_conditional_args('desktop_args', lambda cfg: not self.is_android(cfg))
+    add_conditional_args('lacros_args', self.is_lacros)
+    add_conditional_args('linux_args', self.is_linux)
+    add_conditional_args('android_args', self.is_android)
+    add_conditional_args('chromeos_args', self.is_chromeos)
+    add_conditional_args('mac_args', self.is_mac)
+    add_conditional_args('win_args', self.is_win)
+    add_conditional_args('win64_args', self.is_win64)
+
+    if args_need_fixup[0]:
+      new_test['args'] = self.maybe_fixup_args_array(args)
 
     new_test.update(mixin)
     return new_test

testing/buildbot/generate_buildbot_json_unittest.py

@@ -54,6 +54,12 @@ def regen_test_json(self, fakebb):
     """
     contents = fakebb.generate_outputs()
     with fake_filesystem_unittest.Pause(self.fs):
+      try:
+        os.mkdir(self.output_dir)
+      except FileExistsError:
+        if not os.path.isdir(self.output_dir):
+          raise
+
       fakebb.write_json_result(contents)
 
 
@@ -2582,6 +2588,22 @@ def test_test_suites_must_be_sorted(self):
 }
 """
 
+MIXIN_ARGS = """\
+{
+  'builder_mixin': {
+    'args': [],
+  },
+}
+"""
+
+MIXIN_ARGS_NOT_LIST = """\
+{
+  'builder_mixin': {
+    'args': 'I am not a list',
+  },
+}
+"""
+
 # These mixins are invalid; if passed to check_input_file_consistency, they will
 # fail. These are used for output file consistency checks.
 SWARMING_MIXINS = """\
@@ -2614,6 +2636,21 @@ def test_test_suites_must_be_sorted(self):
 }
 """
 
+SWARMING_NAMED_CACHES = """\
+{
+  'builder_mixin': {
+    'swarming': {
+      'named_caches': [
+        {
+          'name': 'cache',
+          'file': 'cache_file',
+        },
+      ],
+    },
+  },
+}
+"""
+
 SWARMING_MIXINS_APPEND = """\
 {
   'builder_mixin': {
@@ -2977,6 +3014,15 @@ def test_mixin_append_linux_args(self):
     fbb.check_output_file_consistency(verbose=True)
     self.assertFalse(fbb.printed_lines)
 
+  def test_swarming_named_caches(self):
+    fbb = FakeBBGen(self.args,
+                    FOO_GTESTS_BUILDER_MIXIN_WATERFALL,
+                    FOO_TEST_SUITE_WITH_SWARMING_NAMED_CACHES,
+                    LUCI_MILO_CFG,
+                    mixins=SWARMING_NAMED_CACHES)
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
   def test_mixin_append_swarming_named_caches(self):
     fbb = FakeBBGen(self.args,
                     FOO_GTESTS_BUILDER_MIXIN_WATERFALL,
@@ -3010,6 +3056,27 @@ def test_mixin_append_mixin_field_not_list(self):
       fbb.check_output_file_consistency(verbose=True)
     self.assertFalse(fbb.printed_lines)
 
+  def test_args_field_not_list(self):
+    fbb = FakeBBGen(self.args,
+                    FOO_GTESTS_BUILDER_MIXIN_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ARGS,
+                    LUCI_MILO_CFG,
+                    mixins=MIXIN_ARGS_NOT_LIST)
+    with self.assertRaisesRegex(generate_buildbot_json.BBGenErr,
+                                '"args" must be a list'):
+      fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
+  def test_args_field_merging(self):
+    fbb = FakeBBGen(self.args,
+                    FOO_GTESTS_BUILDER_MIXIN_WATERFALL,
+                    FOO_TEST_SUITE_WITH_ARGS,
+                    LUCI_MILO_CFG,
+                    mixins=MIXIN_ARGS)
+    self.regen_test_json(fbb)
+    fbb.check_output_file_consistency(verbose=True)
+    self.assertFalse(fbb.printed_lines)
+
   def test_mixin_append_test_field_not_list(self):
     fbb = FakeBBGen(self.args,
                     FOO_GTESTS_BUILDER_MIXIN_WATERFALL,