Swarming: conditionally run tests on swarming in chromium_trybot recipe.

scripts/slave/recipes/chromium_trybot.py

 # Copyright 2013 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.
 
 DEPS = [
   'bot_update',
   'chromium',
   'gclient',
   'isolate',
   'itertools',
   'json',
   'path',
   'platform',
   'properties',
   'python',
   'raw_io',
   'step',
   'step_history',
+  'swarming',
   'test_utils',
   'tryserver',
 ]
 
 
 BUILDERS = {
   'tryserver.chromium': {
     'builders': {
       'linux_arm_cross_compile': {
         'GYP_DEFINES': {
@@ skipping 401 matching lines @@
           step_test_data=lambda: api.json.test_api.canned_gtest_output(True))
 
     def has_valid_results(self, suffix):
       step_name = self._step_name(suffix)
       gtest_results = api.step_history[step_name].json.gtest_results
       if not gtest_results.valid:  # pragma: no cover
         return False
       global_tags = gtest_results.raw.get('global_tags', [])
       return 'UNRELIABLE_RESULTS' not in global_tags
 
-
     def failures(self, suffix):
       step_name = self._step_name(suffix)
       return api.step_history[step_name].json.gtest_results.failures
 
 
+  class SwarmingGTestTest(api.test_utils.Test):
+    # This test is using 'trigger' and 'collect' instead of 'run'.
+    async = True
+
+    def __init__(self, name, args=None):
+      api.test_utils.Test.__init__(self)
+      self._name = name
+      self._args = args or []
+      self._tasks = {}
+      self._results = {}
+
+    @property
+    def name(self):
+      return self._name
+
+    def compile_targets(self):
+      # <X>_run target depends on <X>, and then isolates it invoking isolate.py.
+      # It is a convention, not a hard coded rule.
+      return [self._name + '_run']
+
+    def trigger(self, suffix):
+      assert suffix not in self._tasks, (
+          'Test %s was already triggered' % self._step_name(suffix))
+
+      # *.isolated may be missing if *_run target is misconfigured. It a error
+      # in gyp, not a recipe failure. So carry on with recipe execution.
+      isolated_hash = api.isolate.isolated_tests.get(self._name)
+      if not isolated_hash:
+        return api.python.inline(
+            '[error] %s' % self._step_name(suffix),
+            r"""
+            import sys
+            print '*.isolated file for target %s is missing' % sys.argv[1]
+            sys.exit(1)
+            """,
+            args=[self._name],
+            always_run=True)
+
+      # If rerunning without a patch, run only tests that failed.
+      args = self._args[:]
+      if suffix == 'without patch':
+        assert self.has_valid_results('with patch')
+        failed_tests = sorted(self.failures('with patch'))
+        args.append('--gtest_filter=%s' % ':'.join(failed_tests))

[Vadim Sh., 2014/06/04 00:03:56]

Local tests are using a temp file with gtest filters, passed to the test via
--test-launcher-filter-file. 

I'm not sure why it is done so, but it's a giant PITA to replicate exact same
behavior on swarming. So reverting to --gtest_filter= for now. I think it should
work.

+
+      # Trigger the test on swarming.
+      self._tasks[suffix] = api.swarming.gtest_task(
+          title=self._step_name(suffix),
+          isolated_hash=isolated_hash,
+          test_launcher_summary_output=api.json.gtest_results(
+              add_json_log=False),
+          extra_args=args)
+      return api.swarming.trigger([self._tasks[suffix]], always_run=True)
+
+    def collect(self, suffix):
+      assert suffix not in self._results, (
+          'Results of %s were already collected' % self._step_name(suffix))
+
+      # Do nothing if test wasn't triggered. This happens if *.isolated is not
+      # found. The build is already red by this moment.
+      if suffix not in self._tasks:
+        return []
+
+      # Update step presentation, store step results in self._results.
+      def followup_fn(step_result):
+        r = step_result.json.gtest_results
+        p = step_result.presentation
+        if r.valid:
+          p.step_text += api.test_utils.format_step_text([
+              ['failures:', r.failures]
+          ])
+        self._results[suffix] = r
+
+      # Wait for test on swarming to finish.
+      return api.swarming.collect(
+          [self._tasks[suffix]],
+          always_run=True,
+          can_fail_build=False,
+          followup_fn=followup_fn)
+
+    def has_valid_results(self, suffix):
+      gtest_results = self._results.get(suffix)
+      if not gtest_results or not gtest_results.valid:  # pragma: no cover
+        return False
+      global_tags = gtest_results.raw.get('global_tags', [])
+      return 'UNRELIABLE_RESULTS' not in global_tags
+
+    def failures(self, suffix):
+      assert suffix in self._results, (
+          'Test %s wasn\'t collected yet' % self._step_name(suffix))
+      return self._results[suffix].failures
+
+
   class NaclIntegrationTest(api.test_utils.Test):  # pylint: disable=W0232
     name = 'nacl_integration'
 
     @staticmethod
     def compile_targets():
       return ['chrome']
 
     def run(self, suffix):
       args = [
         '--mode', api.chromium.c.build_config_fs,
         '--json_build_results_output_file', api.json.output(),
       ]
       return api.python(
           self._step_name(suffix),
           api.path['checkout'].join('chrome',
                             'test',
                             'nacl_test_injection',
                             'buildbot_nacl_integration.py'),
           args,
           can_fail_build=False,
           step_test_data=lambda: api.m.json.test_api.output([]))
 
     def has_valid_results(self, suffix):
       return api.step_history[self._step_name(suffix)].json.output is not None
 
     def failures(self, suffix):
       failures = api.step_history[self._step_name(suffix)].json.output
       return [f['raw_name'] for f in failures]
 
+

[M-A Ruel, 2014/06/04 00:52:53]

remove one empty line

[Vadim Sh., 2014/06/04 02:58:27]

Hmm.. I'm not sure. It marks the end of local classes definitions block and
start of local functions definitions.

+  def parse_test_spec(test_spec, should_use_test):
+    """Returns a list of tests to run and additional targets to compile.

[Vadim Sh., 2014/06/04 00:03:56]

This is refactoring of spec parsing code into a separate function +
should_use_test callback. This callback is also used to decide when to run the
test on swarming.

+
+    Uses 'should_use_test' callback to figure out what tests should be skipped.
+
+    Returns triple (compile_targets, gtest_tests, swarming_tests) where
+      gtest_tests is a list of GTestTest
+      swarming_tests is a list of SwarmingGTestTest.
+    """
+    compile_targets = []
+    gtest_tests_spec = []
+    if isinstance(test_spec, dict):
+      compile_targets = test_spec.get('compile_targets', [])
+      gtest_tests_spec = test_spec.get('gtest_tests', [])
+    else:
+      # TODO (nodir): Remove this after

[M-A Ruel, 2014/06/04 00:52:53]

TODO(nodir):

We never put a space in between.

[Vadim Sh., 2014/06/04 02:58:27]

Done. (I've just copy-pasted it from where it was before).

+      # https://codereview.chromium.org/297303012/#ps50001
+      # lands.
+      gtest_tests_spec = test_spec
+
+    gtest_tests = []
+    swarming_tests = []
+    for test in gtest_tests_spec:
+      test_name = None
+      test_dict = None
+
+      # Read test_dict for the test, it defines where test can run.
+      if isinstance(test, unicode):
+        test_name = test.encode('utf-8')
+        test_dict = {}
+      elif isinstance(test, dict):
+        if 'test' not in test:  # pragma: no cover
+          raise ValueError('Invalid entry in test spec: %r' % test)
+        test_name = test['test'].encode('utf-8')
+        test_dict = test
+      else:  # pragma: no cover
+        raise ValueError('Unrecognized entry in test spec: %r' % test)
+
+      # Should skip it completely?
+      if not test_name or not should_use_test(test_dict):
+        continue
+
+      # If test can run on swarming, test_dict has a section that defines when
+      # swarming should be used, in same format as main test dict.
+      use_swarming = False
+      swarming_spec = test_dict.get('swarming', False)
+      if isinstance(swarming_spec, bool):
+        use_swarming = swarming_spec
+        swarming_spec = {}
+      elif isinstance(swarming_spec, dict):
+        if not swarming_spec:  # pragma: no cover
+          raise ValueError('Empty \'swarming\' section in test spec for %s '
+                           'is ambiguous. Use false or true.' % test_name)

[M-A Ruel, 2014/06/04 00:52:53]

True or False ?

[Vadim Sh., 2014/06/04 02:58:27]

It's originally in *.json file. So boolean literals are lowercase.

+        use_swarming = should_use_test(swarming_spec)
+      else:  # pragma: no cover
+        raise ValueError('Unrecognized swarming entry in test spec: %r' % test)
+
+      test_args = test_dict.get('args')
+      if isinstance(test_args, basestring):

[M-A Ruel, 2014/06/04 00:52:53]

I'm not familiar with this code but the isinstance() dances done in this
function is a bit sad. I hope it'll get away eventually.

[Vadim Sh., 2014/06/04 02:58:27]

I didn't change this logic, it was here previously. Test spec format is a bit
loose. Pawel can comment, since he wrote it (I think).

+        test_args = [test_args]
+
+      if use_swarming:
+        swarming_tests.append(SwarmingGTestTest(test_name, test_args))
+      else:
+        gtest_tests.append(GTestTest(test_name, test_args))
+
+    return compile_targets, gtest_tests, swarming_tests
+
+
   mastername = api.properties.get('mastername')
   buildername = api.properties.get('buildername')
   master_dict = BUILDERS.get(mastername, {})
   bot_config = master_dict.get('builders', {}).get(buildername)
   assert bot_config, (
       'Unrecognized builder name %r for master %r.' % (
           buildername, mastername))
 
   # Make sure tests and the recipe specify correct and matching platform.
   assert api.platform.name == bot_config.get('testing', {}).get('platform')
 
   api.chromium.set_config(bot_config['chromium_config'],
                           **bot_config.get('chromium_config_kwargs', {}))
   # Settings GYP_DEFINES explicitly because chromium config constructor does
   # not support that.
   api.chromium.c.gyp_env.GYP_DEFINES.update(bot_config.get('GYP_DEFINES', {}))
-  if bot_config.get('use_isolate'):
-    api.isolate.set_isolate_environment(api.chromium.c)
   api.chromium.apply_config('trybot_flavor')
   api.gclient.set_config('chromium')
   api.step.auto_resolve_conflicts = True
 
   yield api.bot_update.ensure_checkout()
   # The first time we run bot update, remember if bot_update mode is on or off.
   bot_update_mode = api.step_history.last_step().json.output['did_run']
   if not bot_update_mode:
     yield api.gclient.checkout(
         revert=True, can_fail_build=False, abort_on_failure=False)
@@ skipping 30 matching lines @@
           'args': ['--test-launcher-print-test-stdio=always'],
         },
         {
           'test': 'browser_tests',
           'exclude_builders': ['tryserver.chromium:win_chromium_x64_rel'],
         },
       ]),
       followup_fn=test_spec_followup_fn,
   )
 
+  def should_use_test(test):
+    """Given a test dict from test spec returns True or False."""
+    if 'platforms' in test:

[Vadim Sh., 2014/06/04 00:03:56]

No new logic here.

+      if api.platform.name not in test['platforms']:
+        return False
+    if 'chromium_configs' in test:
+      if bot_config['chromium_config'] not in test['chromium_configs']:
+        return False
+    if 'exclude_builders' in test:
+      if '%s:%s' % (mastername, buildername) in test['exclude_builders']:
+        return False
+    return True
+
+  # Parse test spec file into list of Test instances.
+  compile_targets, gtest_tests, swarming_tests = parse_test_spec(
+      api.step_history['read test spec'].json.output,
+      should_use_test)
+
+  # Swarming uses Isolate to transfer files to swarming bots.
+  # set_isolate_environment modifies GYP_DEFINES to enable test isolation.
+  use_isolate = swarming_tests or bot_config.get('use_isolate')
+  if use_isolate:
+    api.isolate.set_isolate_environment(api.chromium.c)
+
   runhooks_env = bot_config.get('runhooks_env', {})
 
   yield api.chromium.runhooks(env=runhooks_env, abort_on_failure=False,
                               can_fail_build=False)
   if api.step_history.last_step().retcode != 0:
     # Before removing the checkout directory try just using LKCR.
     api.gclient.set_config('chromium_lkcr')
 
     # Since we're likely to switch to an earlier revision, revert the patch,
     # sync with the new config, and apply issue again.
@@ skipping 10 matching lines @@
       if api.step_history.last_step().retcode != 0:
         if api.platform.is_win:
           yield api.chromium.taskkill()
         yield (
           api.path.rmcontents('slave build directory', api.path['slave_build']),
           api.gclient.checkout(revert=False),
           api.tryserver.maybe_apply_issue(),
           api.chromium.runhooks(env=runhooks_env)
         )
 
-  gtest_tests = []
-  compile_targets = []
-  test_spec = api.step_history['read test spec'].json.output
-
-  if isinstance(test_spec, dict):
-    compile_targets = test_spec.get('compile_targets', [])
-    gtest_tests_spec = test_spec.get('gtest_tests', [])
-  else:
-    # TODO (nodir): Remove this after
-    # https://codereview.chromium.org/297303012/#ps50001
-    # lands.
-    gtest_tests_spec = test_spec
-
-  for test in gtest_tests_spec:
-    test_name = None
-    test_args = None
-
-    if isinstance(test, unicode):
-      test_name = test.encode('utf-8')
-    elif isinstance(test, dict):
-      if 'platforms' in test:
-        if api.platform.name not in test['platforms']:
-          continue
-
-      if 'chromium_configs' in test:
-        if bot_config['chromium_config'] not in test['chromium_configs']:
-          continue
-
-      if 'exclude_builders' in test:
-        if '%s:%s' % (mastername, buildername) in test['exclude_builders']:
-          continue
-
-      test_args = test.get('args')
-      if isinstance(test_args, basestring):
-        test_args = [test_args]
-
-      if 'test' not in test:  # pragma: no cover
-        raise ValueError('Invalid entry in test spec: %r' % test)
-
-      test_name = test['test'].encode('utf-8')
-    else:  # pragma: no cover
-      raise ValueError('Unrecognized entry in test spec: %r' % test)
-
-    if test_name:
-      gtest_tests.append(GTestTest(test_name, test_args))
+  # If going to use swarming_client (pinned in src/DEPS), ensure it is
+  # compatible with what recipes expect.
+  if swarming_tests:
+    yield api.swarming.check_client_version()
 
   tests = []
   tests.append(CheckdepsTest())
   tests.append(Deps2GitTest())
-  for test in gtest_tests:
-    tests.append(test)
+  tests.extend(gtest_tests)
+  tests.extend(swarming_tests)
   tests.append(NaclIntegrationTest())
 
   compile_targets.extend(bot_config.get('compile_targets', []))
   compile_targets.extend(api.itertools.chain(
       *[t.compile_targets() for t in tests]))
   # TODO(phajdan.jr): Also compile 'all' on win, http://crbug.com/368831 .
   # Disabled for now because it takes too long and/or fails on Windows.
   if not api.platform.is_win and not bot_config.get('exclude_compile_all'):
     compile_targets = ['all'] + compile_targets
   yield api.chromium.compile(compile_targets,
@@ skipping 33 matching lines @@
       yield api.chromium.runhooks()
       yield api.chromium.compile(compile_targets,
                                  name='compile '
                                       '(with patch, lkcr, clobber, nuke)',
                                  force_clobber=True)
 
   # Do not run tests if the build is already in a failed state.
   if api.step_history.failed:
     return
 
-  if bot_config.get('use_isolate'):
+  # Collect *.isolated hashes for all isolated targets, used when triggering
+  # tests on swarming.
+  if use_isolate:
     yield api.isolate.find_isolated_tests(api.chromium.output_dir)
 
   if bot_config['compile_only']:
     return
 
   if bot_config['chromium_config'] not in ['chromium_chromeos',
                                            'chromium_chromeos_clang']:
     # TODO(phajdan.jr): Make it possible to retry telemetry tests (add JSON).
+    # TODO(vadimsh): Trigger swarming tests before telemetry tests.
     yield (
       api.chromium.run_telemetry_unittests(),
       api.chromium.run_telemetry_perf_unittests(),
     )
 
   def deapply_patch_fn(failing_tests):
     if api.platform.is_win:
       yield api.chromium.taskkill()
     if bot_update_mode:
       yield api.bot_update.ensure_checkout(patch=False,
                                            always_run=True,
                                            update_presentation=False)
     else:
       yield api.gclient.checkout(revert=True, always_run=True),
     yield api.chromium.runhooks(always_run=True),
     compile_targets = list(api.itertools.chain(
                                *[t.compile_targets() for t in failing_tests]))
     if compile_targets:
       yield api.chromium.compile(compile_targets,
                                  name='compile (without patch)',
                                  abort_on_failure=False,
                                  can_fail_build=False,
                                  always_run=True)
       if api.step_history['compile (without patch)'].retcode != 0:
         yield api.chromium.compile(compile_targets,
                                    name='compile (without patch, clobber)',
                                    force_clobber=True,
                                    always_run=True)
+      if use_isolate:
+        yield api.isolate.find_isolated_tests(api.chromium.output_dir,
+                                              always_run=True)
 
   yield api.test_utils.determine_new_failures(tests, deapply_patch_fn)
 
 
 def _sanitize_nonalpha(text):
   return ''.join(c if c.isalnum() else '_' for c in text)
 
 
 def GenTests(api):
   canned_checkdeps = {
@@ skipping 184 matching lines @@
             'test': 'browser_tests',
             'args': '--gtest-filter: *NaCl*',
           }, {
             'test': 'base_tests',
             'args': ['--gtest-filter: *NaCl*'],
           },
         ],
       })
     )
   )
+
+  yield (

[M-A Ruel, 2014/06/04 00:52:53]

I think a comment about why it's done this way would be useful for future
readers. No need to state what is being done but more about the greater context.

[Vadim Sh., 2014/06/04 02:58:27]

Added comments describing what code flow paths are being tested. It's difficult
to explain how this stuff works for readers that are not familiar with recipes
magic :(

+    api.test('swarming_basic') +
+    props() +
+    api.platform.name('linux') +
+    api.override_step_data('read test spec', api.json.output({
+        'gtest_tests': [
+          {
+            'test': 'base_unittests',
+            'swarming': True,

[M-A Ruel, 2014/06/04 00:52:53]

I don't understand why you do not use a dict with the supported platform as done
below. I prefer when the variables do not mutate to different type, especially
in different items in the same list.

[Vadim Sh., 2014/06/04 02:58:27]

It's for code coverage of 'if isinstance(swarming_spec, bool):' branch in
parse_test_spec :) Added explanation to comment. Made all other tests use
'swarming: True'.

+          },
+          {
+            'test': 'browser_tests',
+            'swarming': {
+              'platforms': ['linux'],
+            },
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+        'find isolated tests',
+        api.isolate.output_json(['base_unittests', 'browser_tests']))
+  )
+
+  yield (
+    api.test('swarming_missing_isolated') +
+    props() +
+    api.platform.name('linux') +
+    api.override_step_data('read test spec', api.json.output({
+        'gtest_tests': [
+          {
+            'test': 'base_unittests',
+            'swarming': True,
+          },
+          {
+            'test': 'browser_tests',
+            'swarming': {
+              'platforms': ['linux'],
+            },
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+        'find isolated tests',
+        api.isolate.output_json(['base_unittests']))
+  )
+
+  yield (
+    api.test('swarming_deapply_patch') +
+    props() +
+    api.platform.name('linux') +
+    api.override_step_data('read test spec', api.json.output({
+        'gtest_tests': [
+          {
+            'test': 'base_unittests',
+            'swarming': True,
+          },
+          {
+            'test': 'browser_tests',
+            'swarming': {
+              'platforms': ['linux'],
+            },
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+        'find isolated tests',
+        api.isolate.output_json(['base_unittests', 'browser_tests'])) +
+    api.override_step_data('[swarming] base_unittests (with patch)',
+                           canned_test(passing=False)) +
+    api.override_step_data(
+        'find isolated tests (2)',
+        api.isolate.output_json(['base_unittests', 'browser_tests']))
+  )