Swarming: conditionally run tests on swarming in chromium_trybot recipe.

scripts/slave/recipes/chromium_trybot.py

Index: scripts/slave/recipes/chromium_trybot.py
diff --git a/scripts/slave/recipes/chromium_trybot.py b/scripts/slave/recipes/chromium_trybot.py
index d45b0a66d1ae3891db25b6f3d478379a6d9f3240..ed4ec1a3dbb9dd6d9f6353bd5ee32e4c9c4cd74f 100644
--- a/scripts/slave/recipes/chromium_trybot.py
+++ b/scripts/slave/recipes/chromium_trybot.py
@@ -16,6 +16,7 @@ DEPS = [
   'raw_io',
   'step',
   'step_history',
+  'swarming',
   'test_utils',
   'tryserver',
 ]
@@ -437,12 +438,104 @@ def GenSteps(api):
       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))
+
+      # 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 []

[Paweł Hajdan Jr., 2014/06/04 10:50:31]

Can we issue a failing step just to make sure the build is in fact red?

I'm mostly concerned about some assumptions being no longer true in the future
by accident, and failing here will help catch this.

[Vadim Sh., 2014/06/09 20:27:16]

Done.

+
+      # 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,

[Paweł Hajdan Jr., 2014/06/04 10:50:31]

Could you explain why this is non-fatal? It seems it could result in silent
failures, but maybe I just need to understand better.

[Vadim Sh., 2014/06/09 20:27:16]

For same reason api.chromium.runtest on line 420 (GTestTest.run) is non-fatal.
If I understand correctly, it is related to patch deapply logic. (i.e. red test
doesn't necessary mean red build because trunk may be red).

[Paweł Hajdan Jr., 2014/06/10 08:36:55]

Indeed, the with/without patch steps are non-fatal, and if there is a failure
caused by the patch we emit a failing summary step.

Now that was about running tests locally. With swarming there are many
additional failure modes.

We should indeed ignore the test failures at this point. However, I'd like to
make sure swarming system failures are not ignored, but propagated (similar to
how local test run can yield invalid test results).

Could you add comments here summarizing why test failures are non-fatal in
with/without patch steps (and instead a fatal summary step is issued as needed),
and how are swarming system/infrastructure failures handled?

Note it's not so much about swarming itself - anything that goes over network
can fail, AE might be down, etc.

[Vadim Sh., 2014/06/12 01:00:08]

If swarming itself fails completely, has_valid_results() returns False, because
swarming doesn't produce *.json with test summary in that case.

If only one shard fails... I think right now 'collect' step would be marked as
failed, but it would not fail the build. I'll add 'UNRELIABLE_RESULTS' to
global_tags in that case (in separate a CL). I think it's a good use case for
this.
I don't think it's a right place for that comment, since it is not swarming
related thing. There should be file-level comment (doc string perhaps)
documenting the entire process. I invite you to write it, since you're the
author of that logic (afaik).
 
If swarming\isolate fails, test doesn't produce *.json, and has_valid_results()
returns False. 

But sometimes test run, and fail because a bot is in a stupid state. And such
case is indistinguishable from test failure from swarming standpoint. And it
happens for non-swarming tests as well. So I don't think that such possibility
should block this CL.

+          followup_fn=followup_fn)
+
+    def has_valid_results(self, suffix):

[Paweł Hajdan Jr., 2014/06/04 10:50:31]

Can we also see if self._tasks / self._results look as expected here, i.e. we
have successfully triggered and/or collected these results from swarming?

My intent is to catch various failure modes, and avoid e.g. empty results being
treated as valid/success because collection failed.

Even if that failure mode is not possible now (or we think it's not possible)
I'd like to add defense-in-depth here for the future.

[Vadim Sh., 2014/06/09 20:27:16]

Done.

+      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, (

[Paweł Hajdan Jr., 2014/06/04 10:50:31]

This is an example of a check that I'd recommend to move to has_valid_results.

You could then assert has_valid_results here instead.

[Vadim Sh., 2014/06/09 20:27:16]

Done.

+          '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'
 
@@ -472,6 +565,76 @@ def GenSteps(api):
       failures = api.step_history[self._step_name(suffix)].json.output
       return [f['raw_name'] for f in failures]
 
+
+  def parse_test_spec(test_spec, should_use_test):
+    """Returns a list of tests to run and additional targets to compile.
+
+    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
+      # 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)

[Paweł Hajdan Jr., 2014/06/04 10:50:31]

I strongly recommend different testing strategy.

Could you please introduce a different set of bots to test swarming on?

Then we can compare the pass/fail rate on non-swarming and swarming bots, and
send a percentage of CQ traffic to swarming ones as an experiment (without
blocking CQ on them, we did that already for recipe bots and GPU bots).

[Vadim Sh., 2014/06/09 20:27:16]

Added configs for (linux|mac|win)_chromium_rel_swarming builders, that are
identtical to *_chromium_rel, but with enable_swarming: True. 

If enable_swarming is False (default), swarming is not used (regardless of
what's in test spec). If enable_swarming is True, test spec may specify what
tests to run on swarming.

[Paweł Hajdan Jr., 2014/06/10 08:36:55]

Sounds good to me. Thank you for making this change.

+      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 true or false.' % test_name)
+        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):
+        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, {})
@@ -488,8 +651,6 @@ def GenSteps(api):
   # 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
@@ -540,6 +701,30 @@ def GenSteps(api):
       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:
+      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,
@@ -570,57 +755,16 @@ def GenSteps(api):
           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', []))
@@ -674,7 +818,9 @@ def GenSteps(api):
   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']:
@@ -683,6 +829,7 @@ def GenSteps(api):
   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(),
@@ -711,6 +858,9 @@ def GenSteps(api):
                                    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)
 
@@ -915,3 +1065,81 @@ def GenTests(api):
       })
     )
   )
+
+  # Successfully compiling, isolating and running two targets on swarming.
+  # Check both 'swarming: True' and 'swarming: {dict}' test spec definitions.
+  yield (
+    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,
+          },
+          {
+            'test': 'browser_tests',
+            'swarming': {
+              'platforms': ['linux'],
+            },
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+        'find isolated tests',
+        api.isolate.output_json(['base_unittests', 'browser_tests']))
+  )
+
+  # One target (browser_tests) failed to produce *.isolated file.
+  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': True,
+          },
+        ],
+      })
+    ) +
+    api.override_step_data(
+        'find isolated tests',
+        api.isolate.output_json(['base_unittests']))
+  )
+
+  # One test (base_unittest) failed on swarming. It is retried with
+  # deapplied patch.
+  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': True,
+          },
+        ],
+      })
+    ) +
+    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']))
+  )