[Android] Implement perf tests to platform mode.

build/android/pylib/local/device/local_device_perf_test_run.py

Index: build/android/pylib/local/device/local_device_perf_test_run.py
diff --git a/build/android/pylib/local/device/local_device_perf_test_run.py b/build/android/pylib/local/device/local_device_perf_test_run.py
new file mode 100644
index 0000000000000000000000000000000000000000..ea50269dd70e666567ec556218608dfaa451b936
--- /dev/null
+++ b/build/android/pylib/local/device/local_device_perf_test_run.py
@@ -0,0 +1,375 @@
+# Copyright 2016 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 io
+import itertools
+import json
+import logging
+import os
+import pickle
+import shutil
+import tempfile
+import time
+import zipfile
+
+from devil.android import battery_utils
+from devil.android import device_blacklist
+from devil.android import device_errors
+from devil.android import device_list
+from devil.android import device_utils
+from devil.android import forwarder
+from devil.android.tools import device_recovery
+from devil.android.tools import device_status
+from devil.utils import cmd_helper
+from devil.utils import parallelizer
+from devil.utils import watchdog_timer
+from pylib import constants
+from pylib.base import base_test_result
+from pylib.constants import host_paths
+from pylib.local.device import local_device_test_run
+
+
+class TestShard(object):
+  def __init__(self, env, test_instance, device, index, tests, watcher=None,
+               retries=3, timeout=None):
+    logging.info('Create shard %s for device %s to run the following tests:',
+                 index, device)
+    for t in tests:
+      logging.info('  %s', t)
+    self._battery = battery_utils.BatteryUtils(device)
+    self._device = device
+    self._env = env
+    self._index = index
+    self._output_dir = None
+    self._results = []
+    self._retries = retries
+    self._test_instance = test_instance
+    self._tests = tests
+    self._timeout = timeout
+    self._watcher = watcher
+
+  @local_device_test_run.handle_shard_failures
+  def RunTestsOnShard(self):
+    for test in self._tests:
+      try:
+        tries_left = self._retries
+        result_type = None
+        while (result_type != base_test_result.ResultType.PASS
+               and tries_left > 0):
+          try:
+            self._TestSetUp(test)
+            result_type = self._RunSingleTest(test)
+          except Exception: # pylint: disable=broad-except

[jbudorick, 2016/07/01 14:20:10]

Do we need to catch Exception? Can we catch something narrower? What is
_RunSingleTest throwing -- just devil exceptions?

[rnephew (Reviews Here), 2016/07/01 22:09:32]

I want it to catch just about anything so that it can run device_recovery and
attempt to run again. I could probably narrow it down to just devil exceptions
though; since they are the most likely things.

+            logging.exception('Exception when executing %s.', test)
+            result_type = base_test_result.ResultType.FAIL
+          finally:
+            if result_type != base_test_result.ResultType.PASS:

[jbudorick, 2016/07/01 14:20:10]

Is this the right condition to attempt device recovery, or should we only be
doing it on catching a device exception?

[rnephew (Reviews Here), 2016/07/01 22:09:32]

I think that would not catch the issue where the forwarder is messed up. There
are probably other cases as well.

[jbudorick, 2016/07/06 19:12:09]

Acknowledged. Fine with this for now, but we should keep an eye on it.

+              try:
+                device_recovery.RecoverDevice(self._device, self._env.blacklist)
+              except device_errors.CommandTimeoutError:
+                logging.exception(
+                    'Device failed to recover after failing %s.', test)
+            tries_left = tries_left - 1
+            self._TestTearDown()
+        result = base_test_result.TestRunResults()
+        result.AddResult(base_test_result.BaseTestResult(test, result_type))
+        self._results.append(result)
+      finally:
+        if self._output_dir:

[jbudorick, 2016/07/01 14:20:10]

self._output_dir seems to be handled pretty strangely in this class. It's
deleted here and in _TestSetUp, created in _TestSetUp, and not touched in
_TestTearDown. Ideally, it would be created in _TestSetUp, deleted in
_TestTearDown, and that's it.

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+          shutil.rmtree(self._output_dir, ignore_errors=True)
+          self._output_dir = None
+    return self._results
+
+  def _TestSetUp(self, test):
+    if self._watcher:
+      self._watcher.Reset()
+
+    logging.info('Charge level: %s%%',
+                 str(self._battery.GetBatteryInfo().get('level')))
+    if self._test_instance.min_battery_level:
+      self._battery.ChargeDeviceToLevel(self._test_instance.min_battery_level)
+
+    logging.info('temperature: %s (0.1 C)',
+                 str(self._battery.GetBatteryInfo().get('temperature')))
+    if self._test_instance.max_battery_temp:
+      self._battery.LetBatteryCoolToTemperature(
+          self._test_instance.max_battery_temp)
+
+    if not self._device.IsScreenOn():
+      self._device.SetScreen(True)
+
+    if not self._device.IsOnline():

[jbudorick, 2016/07/01 14:20:10]

Wouldn't we run into issues earlier in this function if this were the case?
Should this be the first device function in _TestSetUp?

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+      msg = 'Device %s is unresponsive.' % str(self._device)
+      raise device_errors.DeviceUnreachableError(msg)
+    if self._output_dir:

[jbudorick, 2016/07/01 14:20:10]

Following from the above, I don't think this should be here. If self._output_dir
isn't None here, then we have a problem w/ the teardown logic.

[rnephew (Reviews Here), 2016/07/01 22:09:31]

Done.

+      shutil.rmtree(self._output_dir)
+    if (self._test_instance.collect_chartjson_data
+        or self._tests[test].get('archive_output_dir')):
+      self._output_dir = tempfile.mkdtemp()
+    if self._watcher:

[jbudorick, 2016/07/01 14:20:10]

self._watcher gets reset twice in here?

Also, the watcher no longer seems to be used at all.

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+      self._watcher.Reset()
+
+  def _RunSingleTest(self, test):
+
+    logging.info('Running %s on shard %d', test, self._index)
+    timeout = (
+        None if self._test_instance.no_timeout

[jbudorick, 2016/07/01 14:20:10]

I *think* this is redundant, as the test run's already checks
test_instance.no_timeout

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+        else self._tests[test].get('timeout', self._timeout))
+    logging.info('Timeout for %s test: %d', test, timeout)
+
+    cmd = self._CreateCmd(test)
+    self._test_instance.WriteBuildBotJson(self._output_dir)
+    cwd = os.path.abspath(host_paths.DIR_SOURCE_ROOT)
+
+    try:
+      logging.debug("Running test with command '%s'", cmd)
+      start_time = time.time()
+      exit_code, output = cmd_helper.GetCmdStatusAndOutputWithTimeout(
+          cmd, timeout, cwd=cwd, shell=True)
+      end_time = time.time()
+      json_output = self._test_instance.ReadChartjsonOutput(self._output_dir)
+    except cmd_helper.TimeoutError as e:

[jbudorick, 2016/07/01 14:20:10]

It'd be good if this could be propagated down to results processing somehow s.t.
the result_type was TIMEOUT instead of the more generic FAIL.

I also think you also need to define end_time in here.

[rnephew (Reviews Here), 2016/07/01 22:09:31]

Done.

+      exit_code = -1
+      output = e.output
+      json_output = ''
+
+    return self._ProcessTestResult(
+        test, cmd, start_time, end_time, exit_code, output, json_output)
+
+  def _CreateCmd(self, test):
+    cmd = '%s --device %s' % (self._tests[test]['cmd'], str(self._device))
+    if self._output_dir:
+      cmd = cmd + ' --output-dir=%s' % self._output_dir
+    if self._test_instance.dry_run:
+      cmd = 'echo %s' % cmd
+    return cmd
+

[jbudorick, 2016/07/01 14:20:10]

nit: -1 line

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+
+  def _ProcessTestResult(
+      self, test, cmd, start_time, end_time, exit_code, output, json_output):
+    if exit_code is None:
+      exit_code = -1
+    logging.info('%s : exit_code=%d in %d secs on device %s',
+                 test, exit_code, end_time - start_time,
+                 str(self._device))
+    if exit_code == 0:
+      result_type = base_test_result.ResultType.PASS
+    else:
+      result_type = base_test_result.ResultType.FAIL
+    actual_exit_code = exit_code
+    if (self._test_instance.flaky_steps
+        and test in self._test_instance.flaky_steps):
+      exit_code = 0
+    archive_bytes = (self._ArchiveOutputDir()
+                     if self._tests[test].get('archive_output_dir')
+                     else None)
+    persisted_result = {
+        'name': test,
+        'output': [output],
+        'chartjson': json_output,
+        'archive_bytes': archive_bytes,
+        'exit_code': exit_code,
+        'actual_exit_code': actual_exit_code,
+        'result_type': result_type,
+        'start_time': start_time,
+        'end_time': end_time,
+        'total_time': end_time - start_time,
+        'device': str(self._device),
+        'cmd': cmd,
+    }
+    self._SaveResult(persisted_result)
+    return result_type
+
+  def _ArchiveOutputDir(self):
+    """Archive all files in the output dir, and return as compressed bytes."""
+    with io.BytesIO() as archive:
+      with zipfile.ZipFile(archive, 'w', zipfile.ZIP_DEFLATED) as contents:
+        num_files = 0
+        for absdir, _, files in os.walk(self._output_dir):
+          reldir = os.path.relpath(absdir, self._output_dir)
+          for filename in files:
+            src_path = os.path.join(absdir, filename)
+            # We use normpath to turn './file.txt' into just 'file.txt'.
+            dst_path = os.path.normpath(os.path.join(reldir, filename))
+            contents.write(src_path, dst_path)
+            num_files += 1
+      if num_files:
+        logging.info('%d files in the output dir were archived.', num_files)
+      else:
+        logging.warning('No files in the output dir. Archive is empty.')
+      return archive.getvalue()
+
+  @staticmethod
+  def _SaveResult(result):
+    pickled = os.path.join(constants.PERF_OUTPUT_DIR, result['name'])
+    if os.path.exists(pickled):
+      with file(pickled, 'r') as f:
+        previous = pickle.loads(f.read())
+        result['output'] = previous['output'] + result['output']
+    with file(pickled, 'w') as f:
+      f.write(pickle.dumps(result))
+
+  def _TestTearDown(self):
+    try:
+      logging.info('Unmapping device ports for %s.', self._device)
+      forwarder.Forwarder.UnmapAllDevicePorts(self._device)
+    except Exception: # pylint: disable=broad-except
+      logging.exception('Exception when resetting ports.')
+
+
+class LocalDevicePerfTestRun(local_device_test_run.LocalDeviceTestRun):
+  def __init__(self, env, test_instance):
+    super(LocalDevicePerfTestRun, self).__init__(env, test_instance)
+    self._test_instance = test_instance

[jbudorick, 2016/07/01 14:20:10]

nit: alpha

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+    self._env = env
+    self._timeout = None if test_instance.no_timeout else 60 * 60
+    self._devices = None
+    self._test_buckets = []
+    self._watcher = None
+
+  def SetUp(self):
+    self._devices = self._GetAllDevices(self._env.devices,
+                                        self._test_instance.known_devices_file)
+    self._watcher = watchdog_timer.WatchdogTimer(self._timeout)

[jbudorick, 2016/07/01 14:20:10]

again, I don't think this is used any more.

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+
+    if (not (self._test_instance.print_step

[jbudorick, 2016/07/01 14:20:10]

Do you still need this if check? Neither of these should be true, right?

[rnephew (Reviews Here), 2016/07/01 22:09:31]

Done.

+             or self._test_instance.output_json_list)):
+      if os.path.exists(constants.PERF_OUTPUT_DIR):
+        shutil.rmtree(constants.PERF_OUTPUT_DIR)
+      os.makedirs(constants.PERF_OUTPUT_DIR)
+
+  def TearDown(self):
+    pass
+
+  def _GetStepsFromDict(self):
+    # From where this is called one of these two must be set.

[jbudorick, 2016/07/01 14:20:10]

If this is the case, we should fail hard in the event that it's not true. As
currently written, _SplitTestsByAffinity would hit an unclear error when trying
to do test_dict['steps'].iteritems() w/ test_dict = None.

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Done.

+    if self._test_instance.single_step:
+      return {
+          'version': 1,
+          'steps': {
+              'single_step': {
+                'device_affinity': 0,
+                'cmd': self._test_instance.single_step
+              },
+          }
+      }
+    if self._test_instance.steps:
+      with file(self._test_instance.steps, 'r') as f:
+        steps = json.load(f)
+        if steps['version'] != 1:
+          raise TestDictVersionError(
+              'Version is expected to be %d but was %d' % (1, steps['version']))
+        return steps
+
+  def _SplitTestsByAffinity(self):
+    test_dict = self._GetStepsFromDict()
+    for test, test_config in test_dict['steps'].iteritems():
+      try:
+        affinity = test_config['device_affinity']
+        if len(self._test_buckets) < affinity + 1:
+          while len(self._test_buckets) != affinity + 1:
+            self._test_buckets.append({})
+        self._test_buckets[affinity][test] = test_config
+      except KeyError:
+        logging.exception('Bad test config')

[jbudorick, 2016/07/01 14:20:10]

This should include more detail -- at least the bad test config itself, if not
exactly what lookup failed

[rnephew (Reviews Here), 2016/07/01 22:09:31]

Done.

+    return self._test_buckets
+
+  @staticmethod
+  def _GetAllDevices(active_devices, devices_path):
+    try:
+      if devices_path:
+        devices = [device_utils.DeviceUtils(s)
+                   for s in device_list.GetPersistentDeviceList(devices_path)]
+        if not devices and active_devices:
+          logging.warning('%s is empty. Falling back to active devices.',
+                          devices_path)
+          devices = active_devices
+      else:
+        logging.warning('Known devices file path not being passed. For device '
+                        'affinity to work properly, it must be passed.')
+        devices = active_devices
+    except IOError as e:
+      logging.error('Unable to find %s [%s]', devices_path, e)
+      devices = active_devices
+    return sorted(devices)
+
+  def RunTests(self):
+    # Affinitize the tests.
+    test_buckets = self._SplitTestsByAffinity()
+    if not test_buckets:
+      raise local_device_test_run.NoTestsError()
+
+    blacklist = (device_blacklist.Blacklist(self._env.blacklist)
+                 if self._env.blacklist
+                 else None)
+
+    def run_perf_tests(x):
+      if device_status.IsBlacklisted(str(self._devices[x]), blacklist):
+        logging.warning('Device %s is not active. Will not create shard %s.',
+                        str(self._devices[x]), x)
+        return []
+      s = TestShard(self._env, self._test_instance, self._devices[x], x,
+                    test_buckets[x], watcher=self._watcher,
+                    retries=self._env.max_tries, timeout=self._timeout)
+      return s.RunTestsOnShard()
+
+    device_indices = range(min(len(self._devices), len(test_buckets)))
+    shards = parallelizer.Parallelizer(device_indices).pMap(run_perf_tests)
+    return list(itertools.chain.from_iterable(shards.pGet(self._timeout)))

[jbudorick, 2016/07/01 14:20:10]

🔥

[rnephew (Reviews Here), 2016/07/01 22:09:32]

Acknowledged.

+
+  # override
+  def TestPackage(self):
+    return 'perf'
+
+  # override
+  def _CreateShards(self, _tests):
+    raise NotImplementedError
+
+  # override
+  def _GetTests(self):
+    return self._test_buckets
+
+  # override
+  def _RunTest(self, _device, _test):
+    raise NotImplementedError
+
+  # override
+  def _ShouldShard(self):
+    return False
+
+
+class LocalDevicePerfTestRunOutputJsonList(LocalDevicePerfTestRun):
+  def SetUp(self):
+    pass
+
+  def RunTests(self):
+    return self._test_instance.RunOutputJsonList()
+
+  # override
+  def _CreateShards(self, _tests):

[jbudorick, 2016/07/01 14:20:10]

Do these need to be here?

[rnephew (Reviews Here), 2016/07/01 22:09:31]

Yep. Linter errors.

+    raise NotImplementedError
+
+  # override
+  def _RunTest(self, _device, _test):
+    raise NotImplementedError
+
+
+class LocalDevicePerfTestRunPrintStep(LocalDevicePerfTestRun):
+  def SetUp(self):
+    pass
+
+  def RunTests(self):
+    return self._test_instance.RunPrintStep()
+
+  # override
+  def _CreateShards(self, _tests):
+    raise NotImplementedError
+
+  # override
+  def _RunTest(self, _device, _test):
+    raise NotImplementedError
+
+
+class TestDictVersionError(Exception):
+  pass