Add swarming support for IsolatedScriptTest.

scripts/slave/recipe_modules/chromium_tests/steps.py

 # Copyright 2014 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.
 
 import re
 import string
 
 
 class Test(object):
   """
@@ skipping 857 matching lines @@
     isolate_file_path = (api.path['checkout'].join(self._android_isolate_path)
                          if self._android_isolate_path else None)
     return AndroidInstrumentationTest(
         name=self.name,
         compile_target=self._compile_target,
         apk_under_test=self._apk_under_test,
         test_apk=self.name,
         isolate_file_path=isolate_file_path).run(api, suffix)
 
 
-class IsolatedScriptTest(Test):
+class LocalIsolatedScriptTest(Test):
   def __init__(self, name, args=None, target_name=None, **runtest_kwargs):
-    """Constructs an instance of IsolatedScriptTest.
+    """Constructs an instance of LocalIsolatedScriptTest.
 
-    An IsolatedScriptTest knows how to invoke an isolate which obeys a certain
+    An LocalIsolatedScriptTest knows how to invoke an isolate which obeys a certain
     contract. The isolate's main target must be a wrapper script which must
     interpret certain command line arguments as follows:
 
       --isolated-script-test-output [FILENAME]
 
     The wrapper script must write the simplified json output that the recipes
     consume (similar to GTestTest and ScriptTest) into |FILENAME|.
 
     The contract may be expanded later to support functionality like sharding
     and retries of specific failed tests. Currently the examples of such wrapper
     scripts live in src/testing/scripts/ in the Chromium workspace.
 
     Args:
       name: Displayed name of the test. May be modified by suffixes.
       args: Arguments to be passed to the test.
       target_name: Actual name of the test. Defaults to name.
       runtest_kwargs: Additional keyword args forwarded to the runtest.
     """
-    super(IsolatedScriptTest, self).__init__()
+    super(LocalIsolatedScriptTest, self).__init__()
     self._name = name
     self._args = args or []
     self._target_name = target_name
     self._runtest_kwargs = runtest_kwargs
 
   @property
   def name(self):
     return self._name
 
   @property
@@ skipping 50 matching lines @@
       self.failures(api, suffix)
 
       return self._test_runs[suffix].json.output['valid']
     except Exception:  # pragma: no cover
       return False
 
   def failures(self, api, suffix):
     return self._test_runs[suffix].json.output['failures']
 
 
+class SwarmingIsolatedScriptTest(SwarmingTest):
+  def __init__(self, name, args=None, target_name=None, shards=1,
+               dimensions=None, tags=None, extra_suffix=None,
+               upload_test_results=True):
+    super(SwarmingIsolatedScriptTest, self).__init__(
+        name, dimensions, tags, target_name, extra_suffix)
+    self._args = args or []
+    self._shards = shards
+    self._upload_test_results = upload_test_results
+
+  @property
+  def target_name(self):
+    return self._target_name or self._name
+
+  def compile_targets(self, _):
+    return [self.target_name]
+
+  @property
+  def uses_swarming(self):
+    return True
+
+  def create_task(self, api, suffix, isolated_hash):
+    # For local tests args are added inside api.chromium.run_telemetry_test.
+    browser_config = api.chromium.c.build_config_fs.lower()
+    args = self._args[:]
+
+    # TODO(nednguyen): only rerun the tests that failed.
+    if suffix == 'without patch':  # pragma: no cover

[Paweł Hajdan Jr., 2015/10/26 17:00:40]

Whoa, retrying everything would be much better than not doing anything.

[nednguyen, 2015/10/27 00:54:29]

But not doing anything here mean we gonna rerun all the tests?

[Paweł Hajdan Jr., 2015/10/27 11:12:52]

Oh, I realized it's "pass" and not "return". Sorry, it makes no sense to me.

TODO sounds good, but please remove the noop "if".

[nednguyen, 2015/10/27 16:36:48]

Done.

+      pass
+
+    # For the time being, we assume all isolated_script_test are not idempotent
+    # TODO(nednguyen): make this configurable in isolated_scripts's spec.
+    return api.swarming.isolated_script_task(
+        title=self._step_name(suffix), isolated_hash=isolated_hash,
+        idempotent=None, extra_args=args)

[Paweł Hajdan Jr., 2015/10/26 17:00:41]

Why None as opposed to e.g. False?

[nednguyen, 2015/10/27 00:54:29]

Done.

+
+  def validate_task_results(self, api, step_result):
+    results = getattr(step_result, 'isolated_script_results', None) or {}
+
+    try:
+      failures = results['failures']
+      if not failures and step_result.retcode != 0:
+        failures = ['%s (entire test suite)' % self.name]

[Paweł Hajdan Jr., 2015/10/26 17:00:40]

I think this means valid=False.

Otherwise, if e.g. the entire test suite is broken like that on HEAD, the recipe
would start PASSING try jobs that break it even further.

[Ken Russell (switch to Gerrit), 2015/10/26 18:55:43]

I'm not sure about that. Please see https://codereview.chromium.org/1372723002
which fixed a major problem where totally broken test runs were treated as
passing. I think valid needs to be True in this case.

[Paweł Hajdan Jr., 2015/10/27 11:12:52]

Yup, returning no failures where there are in fact failures sounds like a bug.
In fact, we might still have some holes when e.g. exit code is non-zero but the
code claims there were no failures. Feel free to file a bug to make the code
more robust for that case.
Could you explain more? Do we ever treat valid=False as success? That would
sound like a terrible bug to me.

[Ken Russell (switch to Gerrit), 2015/10/27 12:55:55]

Looking again at https://codereview.chromium.org/1372723002 and remembering some
of the earlier symptoms, leaving valid=true in this case will display the fact
that the entire test suite failed (due to an exception, for example) rather than
"TEST RESULTS WERE INVALID". I think it's clearer to explicitly say that the
entire test suite failed, but don't feel that strongly about it.

[nednguyen, 2015/10/27 16:36:48]

Set "valid=False" here.

+      valid = results['valid']
+    except (ValueError, KeyError) as e:
+      step_result.presentation.logs['invalid_results_exc'] = [str(e)]
+      valid = False
+      failures = None
+    if valid:
+      step_result.presentation.step_text += api.test_utils.format_step_text([
+        ['failures:', failures]
+      ])
+    return valid, failures
+
+
+class IsolatedScriptTest(Test):

[Paweł Hajdan Jr., 2015/10/26 17:00:41]

What's the reason for introducing another indirection layer here?

Existing code followed this pattern because there was a use case that no longer
exists.

[Ken Russell (switch to Gerrit), 2015/10/26 18:55:43]

It'd be fine IMO to remove this indirection, FWIW.

[nednguyen, 2015/10/27 00:54:29]

Done.

+  def __init__(self, name, args=None, target_name=None, enable_swarming=False,
+               swarming_shards=1, swarming_dimensions=None, swarming_tags=None,
+               swarming_extra_suffix=None, **runtest_kwargs):
+    super(IsolatedScriptTest, self).__init__()
+    if enable_swarming:
+      self._test = SwarmingIsolatedScriptTest(
+          name, args, target_name, swarming_shards, swarming_dimensions,
+          swarming_tags, swarming_extra_suffix)
+    else:
+      self._test = LocalIsolatedScriptTest(name, args, target_name,
+                                           **runtest_kwargs)
+
+  @property
+  def name(self):
+    return self._test.name
+
+  @property
+  def uses_local_devices(self):
+    return True  # pragma: no cover
+
+  @property
+  def isolate_target(self):
+    return self._test.isolate_target
+
+  def compile_targets(self, api):
+    return self._test.compile_targets(api)
+
+  def pre_run(self, api, suffix):
+    return self._test.pre_run(api, suffix)
+
+  def run(self, api, suffix):
+    return self._test.run(api, suffix)
+
+  def post_run(self, api, suffix):
+    return self._test.post_run(api, suffix)
+
+  def has_valid_results(self, api, suffix):
+    return self._test.has_valid_results(api, suffix)  # pragma: no cover
+
+  def failures(self, api, suffix):
+    return self._test.failures(api, suffix)  # pragma: no cover
+
+  @property
+  def uses_swarming(self):
+    return self._test.uses_swarming
+
+
 def generate_isolated_script(api, mastername, buildername, test_spec,
                              enable_swarming=False,
                              scripts_compile_targets=None):
-  for spec in test_spec.get(buildername, {}).get(
-      'isolated_scripts', []):
+  for spec in test_spec.get(buildername, {}).get('isolated_scripts', []):
+    use_swarming = False
+    swarming_shards = 1
+    if enable_swarming:
+      swarming_spec = spec.get('swarming', {})
+      if swarming_spec.get('can_use_on_swarming_builders'):
+        use_swarming = True
+        swarming_shards = swarming_spec.get('shards', 1)
+
     yield IsolatedScriptTest(
         str(spec['name']),
         args=spec.get('args', []),
-        target_name=spec['isolate_name'])
+        target_name=spec['isolate_name'],
+        enable_swarming=use_swarming,
+        swarming_shards=swarming_shards)
 
 
 class GTestTest(Test):
   def __init__(self, name, args=None, target_name=None, enable_swarming=False,
                swarming_shards=1, swarming_dimensions=None, swarming_tags=None,
                swarming_extra_suffix=None, **runtest_kwargs):
     super(GTestTest, self).__init__()
     if enable_swarming:
       self._test = SwarmingGTestTest(
           name, args, target_name, swarming_shards, swarming_dimensions,
@@ skipping 706 matching lines @@
   def run(self, api, suffix):
     api.chromium_android.coverage_report(upload=False)
     api.chromium_android.get_changed_lines_for_revision()
     api.chromium_android.incremental_coverage_report()
 
 
 GOMA_TESTS = [
   GTestTest('base_unittests'),
   GTestTest('content_unittests'),
 ]