Move screenshot capture to Java-side.

build/android/pylib/local/device/local_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.
 
 import logging
 import os
 import posixpath
 import re
 import time
 
@@ skipping 368 matching lines @@
     #   - clearing the application state while persisting permissions
     if any(r.GetType() not in (base_test_result.ResultType.PASS,
                                base_test_result.ResultType.SKIP)
            for r in results):
       with contextlib_ext.Optional(
           tempfile_ext.NamedTemporaryDirectory(),
           self._test_instance.screenshot_dir is None and
               self._test_instance.gs_results_bucket) as screenshot_host_dir:
         screenshot_host_dir = (
             self._test_instance.screenshot_dir or screenshot_host_dir)
+
         if screenshot_host_dir:
-          file_name = '%s-%s.png' % (
-              test_display_name,
-              time.strftime('%Y%m%dT%H%M%S-UTC', time.gmtime()))
-          screenshot_file = device.TakeScreenshot(
-              os.path.join(screenshot_host_dir, file_name))
-          logging.info(
-              'Saved screenshot for %s to %s.',
-              test_display_name, screenshot_file)
-          if self._test_instance.gs_results_bucket:
-            link = google_storage_helper.upload(
-                google_storage_helper.unique_name('screenshot', device=device),
-                screenshot_file,
-                bucket=self._test_instance.gs_results_bucket + '/screenshots')
-            for result in results:
-              result.SetLink('post_test_screenshot', link)
+          screenshot_device_file = posixpath.join(
+              device.GetExternalStoragePath(),
+              'chromium_tests_root',

[jbudorick, 2017/05/05 01:21:58]

If I don't like hard-coding things, I *really* don't like hard-coding things in
multiple places.

This depends on the implementation of getIsolatedTestFilePath referring to
*exactly* <external storage>/chromium_tests_root *and* on
ScreenshotOnFailureStatement concatenating the provided --screenshot-dir to that
path. Can we reduce the number of places in which this is hard-coded? Maybe we
can have the script tell the Statement exactly where it should write the
screenshot?

[mikecase (-- gone --), 2017/05/05 18:09:19]

Changed --screenshot-dir arg to just --screenshot-path so fewer things have to
be hard-coded.

Currently, other things depend on getIsolatedTestFilePath referring to exactly
<external storage>/chromium_tests_root....at least I think so. Like, we push the
data to the device to chromium_tests_root, and then use UrlUtils to find the
test data.

+              instrumentation_test_instance.FAILURE_SCREENSHOT_SUBDIR,
+              'failure.png')
+          screenshot_host_file = os.path.join(
+              screenshot_host_dir,
+              '%s-%s.png' % (
+                  test_display_name,
+                  time.strftime('%Y%m%dT%H%M%S-UTC', time.gmtime())))
+          if device.FileExists(screenshot_device_file):
+            device.PullFile(screenshot_device_file, screenshot_host_file)
+            device.RemovePath(screenshot_device_file)

[jbudorick, 2017/05/05 01:21:58]

We should try to remove the file even if pulling it fails for some reason. Maybe
a try/finally here?

[mikecase (-- gone --), 2017/05/05 18:09:19]

sounds good. Done

+
+            logging.info(
+                'Saved screenshot for %s to %s.',
+                test_display_name, screenshot_host_file)
+            if self._test_instance.gs_results_bucket:
+              link = google_storage_helper.upload(
+                  google_storage_helper.unique_name(
+                      'screenshot', device=device),
+                  screenshot_host_file,
+                  bucket=self._test_instance.gs_results_bucket + '/screenshots')

[jbudorick, 2017/05/05 01:21:58]

nit: '%s/screenshots' % self._test_instance.gs_results_bucket

+              for result in results:
+                result.SetLink('post_test_screenshot', link)
 
       logging.info('detected failure in %s. raw output:', test_display_name)
       for l in output:
         logging.info('  %s', l)
       if (not self._env.skip_clear_data
           and self._test_instance.package_info):
         permissions = (
             self._test_instance.apk_under_test.GetPermissions()
             if self._test_instance.apk_under_test
             else None)
@@ skipping 55 matching lines @@
       if k in annotations:
         timeout = v
         break
     else:
       logging.warning('Using default 1 minute timeout for %s', test_name)
       timeout = 60
 
     timeout *= cls._GetTimeoutScaleFromAnnotations(annotations)
 
     return timeout