[Android] Add sharding for AMP instrumentation tests.

build/android/pylib/remote/device/remote_device_instrumentation_test_run.py

 # Copyright 2015 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.
 
 """Run specific test on specific environment."""
 
 import logging
 import os
 import tempfile
 
 from pylib.base import base_test_result
 from pylib.remote.device import remote_device_test_run
 
 
 class RemoteDeviceInstrumentationTestRun(
     remote_device_test_run.RemoteDeviceTestRun):
   """Run instrumentation tests on a remote device."""
 
   #override
+  def _GetAdditionalApks(self):

[rnephew (Reviews Here), 2015/10/26 20:42:18]

It looks like this is returning the test_apk as well as additional_apks. This
name doesn't convey that very well. _GetAllApks() might be a little clearer? But
then its still missing apk_under_test, so maybe not.

[mikecase (-- gone --), 2015/10/27 01:27:43]

Yeah, I don't like this either. So there is an --additional-apk argument to
test_runner.py for "additional apks you need to install on appurify for your
test to run." This is the list in self._test_instance.additional_apks. However,
we need to add test_apk to this list before passing it to the 'additional_apks'
appurify config. I deviced to add this new method. I'll try to think up a better
name.

+    return [self._test_instance.test_apk] + self._test_instance.additional_apks
+
+  #override
+  def _GetAppPath(self):
+    return self._test_instance.apk_under_test
+
+  #override
+  def _GetTestFramework(self):
+    if self._env.test_framework:
+      logging.warning('Ignoring configured test_framework "%s"',
+                      self._env.test_framework)
+    return 'robotium'
+
+  #override
+  def _GetTestPath(self):
+    return self._test_instance.driver_apk
+
+  #override
+  def _GetTestRunnerName(self):

[rnephew (Reviews Here), 2015/10/26 20:42:18]

Is this used anywhere else than _GetFrameworkConfigs? If not maybe it could just
be merged with that.

[mikecase (-- gone --), 2015/10/27 01:27:43]

Merged!

+    return self._test_instance.driver_name
+
+  #override
+  def _GetFrameworkConfigs(self):
+    return {'runner': self._GetTestRunnerName()}
+
+  # pylint: disable=protected-access

[rnephew (Reviews Here), 2015/10/26 20:42:18]

Just an idea, feel free to ignore. 
Instead of disabling the pylint check, you could just add a AddDataDeps([list of
deps]) function to the test_instance. 

[mikecase (-- gone --), 2015/10/27 01:27:43]

We decided offline to keep as is.

+  #override
+  def _GetShardEnvironmentVars(self, num_shards):
+    """Returns a list of shard specfic environment configs."""
+    all_tests = self._test_instance.GetTests()
+    test_shards = [all_tests[i::num_shards] for i in range(num_shards)]
+
+    shard_env_vars = []
+    for i, tests in enumerate(test_shards):
+      test_list_file = tempfile.NamedTemporaryFile(

[jbudorick, 2015/10/26 22:23:30]

Again, not a huge fan of this. Restructure the code s.t. you can take advantage
of a context manager to handle the temporary file's lifetime.

[mikecase (-- gone --), 2015/10/27 01:27:43]

Using a context manager is super awkward when you have to make #(num_shard)
number of tempfiles.
Also, I thought how it was set up before was really hard to follow...

self.SetUp()
  self._TriggerSetup()
  with test_list_file:
    self._AmInstrumentTestSetup()
      with test_zipfile:
        self._SetTestConfig()
          with test_config_file:
            self.UploadConfig()

It got super deep because everything was in a context manager. Like uploading a
config was done within 3 context manager in 3 functions. I prefer flat code over
nested code.

+          suffix='.txt', dir=self._base_tempfile_dir, delete=False)
+      self._test_instance._data_deps.append(
+          (os.path.abspath(test_list_file.name), None))
+      for t in tests:
+        test_name = '%s#%s' % (t['class'], t['method'])
+        logging.debug('shard %s: %s', i, test_name)
+        test_list_file.write('%s\n' % test_name)
+      test_list_file.flush()
+
+      env_vars = self._test_instance.GetDriverEnvironmentVars(
+          test_list_file_path=test_list_file.name)
+      env_vars.update(self._test_instance.GetHttpServerEnvironmentVars())
+      shard_env_vars.append(env_vars)
+
+    return shard_env_vars
+
+  #override
+  def _ShouldShard(self):
+    return True
+
+  #override
   def TestPackage(self):
     return self._test_instance.test_package
 
   #override
-  def _TriggerSetUp(self):
-    """Set up the triggering of a test run."""
-    logging.info('Triggering test run.')
-
-    # pylint: disable=protected-access
-    with tempfile.NamedTemporaryFile(suffix='.txt') as test_list_file:
-      tests = self._test_instance.GetTests()
-      logging.debug('preparing to run %d instrumentation tests remotely:',
-                    len(tests))
-      for t in tests:
-        test_name = '%s#%s' % (t['class'], t['method'])
-        logging.debug('  %s', test_name)
-        test_list_file.write('%s\n' % test_name)
-      test_list_file.flush()
-      self._test_instance._data_deps.append(
-          (os.path.abspath(test_list_file.name), None))
-
-      env_vars = self._test_instance.GetDriverEnvironmentVars(
-          test_list_file_path=test_list_file.name)
-      env_vars.update(self._test_instance.GetHttpServerEnvironmentVars())
-
-      logging.debug('extras:')
-      for k, v in env_vars.iteritems():
-        logging.debug('  %s: %s', k, v)
-
-      self._AmInstrumentTestSetup(
-          self._test_instance.apk_under_test,
-          self._test_instance.driver_apk,
-          self._test_instance.driver_name,
-          environment_variables=env_vars,
-          extra_apks=([self._test_instance.test_apk] +
-                      self._test_instance.additional_apks))
-
-  #override
-  def _ParseTestResults(self):
+  def _ParseTestResults(self, test_output, results_zip):
     logging.info('Parsing results from stdout.')
-    r = base_test_result.TestRunResults()
+    results = base_test_result.TestRunResults()
     result_code, result_bundle, statuses = (
         self._test_instance.ParseAmInstrumentRawOutput(
-            self._results['results']['output'].splitlines()))
+            test_output['results']['output'].splitlines()))
     result = self._test_instance.GenerateTestResults(
         result_code, result_bundle, statuses, 0, 0)
 
     if isinstance(result, base_test_result.BaseTestResult):
-      r.AddResult(result)
+      results.AddResult(result)
     elif isinstance(result, list):
-      r.AddResults(result)
+      results.AddResults(result)
     else:
       raise Exception('Unexpected result type: %s' % type(result).__name__)
 
-    self._DetectPlatformErrors(r)
-    return r
+    self._DetectPlatformErrors(results, test_output, results_zip)
+    return results