Add failure screenshots and render test images to results detail.

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

Index: build/android/pylib/local/device/local_device_instrumentation_test_run.py
diff --git a/build/android/pylib/local/device/local_device_instrumentation_test_run.py b/build/android/pylib/local/device/local_device_instrumentation_test_run.py
index 4b3f353ba6feefcda60f4d5335ed094223f4b551..b38728f90a57b062b7eb441824cb3dafa5c99ff3 100644
--- a/build/android/pylib/local/device/local_device_instrumentation_test_run.py
+++ b/build/android/pylib/local/device/local_device_instrumentation_test_run.py
@@ -6,6 +6,8 @@ import logging
 import os
 import posixpath
 import re
+import shutil
+import tempfile
 import time
 
 from devil.android import device_errors
@@ -13,6 +15,7 @@ from devil.android import flag_changer
 from devil.utils import reraiser_thread
 from pylib import valgrind_tools
 from pylib.android import logdog_logcat_monitor
+from pylib.constants import host_paths
 from pylib.base import base_test_result
 from pylib.instrumentation import instrumentation_test_instance
 from pylib.local.device import local_device_environment
@@ -34,6 +37,15 @@ TIMEOUT_ANNOTATIONS = [
   ('SmallTest', 1 * 60),
 ]
 
+_RE_RENDER_IMAGE_NAME = re.compile(
+      r'(?P<test_class>\w+)\.'
+      r'(?P<description>\w+)\.'
+      r'(?P<device_model>\w+)\.'
+      r'(?P<orientation>port|land)\.png')
+
+RENDER_TESTS_RESULTS_DIR = {
+  'chrome_public_test_apk': 'chrome/test/data/android/render_tests'
+}
 
 # TODO(jbudorick): Make this private once the instrumentation test_runner is
 # deprecated.
@@ -295,6 +307,56 @@ class LocalDeviceInstrumentationTestRun(
       if logcat_url:
         result.SetLink('logcat', logcat_url)
 
+    # Save render test results.
+    if self._test_instance.should_save_images:

[jbudorick, 2017/02/21 18:37:12]

Also, would it make sense to extract this into its own function?

[mikecase (-- gone --), 2017/02/23 00:21:26]

Done

+      if self._test_instance.suite in RENDER_TESTS_RESULTS_DIR:
+        render_results_dir = RENDER_TESTS_RESULTS_DIR[self._test_instance.suite]
+
+        temp_dir = None

[jbudorick, 2017/02/21 18:32:40]

https://chromium.googlesource.com/external/github.com/catapult-project/catapu...

[mikecase (-- gone --), 2017/02/23 00:21:26]

Done

+        try:
+          temp_dir = tempfile.mkdtemp()
+
+          failure_images_device_dir = posixpath.join(
+              device.GetExternalStoragePath(),
+              'chromium_tests_root', render_results_dir, 'failures')
+          device.PullFile(failure_images_device_dir, temp_dir)
+          device.RemovePath(failure_images_device_dir)
+
+          for failure_filename in os.listdir(

[jbudorick, 2017/02/21 18:37:12]

Of note, though: process_render_test_results attempts to compute the image diff,
while this does not. That seems like it'd be useful functionality to preserve.

[mikecase (-- gone --), 2017/02/23 00:21:26]

Added diffing back. Makes this a bit more complex.

+              os.path.join(temp_dir, 'failures')):
+
+            m = _RE_RENDER_IMAGE_NAME.match(failure_filename)

[jbudorick, 2017/02/21 18:32:39]

Do you do anything with m other than check it here?

[mikecase (-- gone --), 2017/02/23 00:21:26]

no

+            if not m:
+              logging.warning('Unexpected file in render test failures, %s',

[jbudorick, 2017/02/21 18:32:39]

nit: "... failures: %s"
                  ^

[mikecase (-- gone --), 2017/02/23 00:21:26]

Done

+                              failure_filename)
+              continue
+
+            failure_filepath = os.path.join(
+                temp_dir, 'failures', failure_filename)
+
+            link = logdog_helper.image(
+                logdog_helper.unique_name(failure_filename, device=device),
+                failure_filepath)
+            for result in results:
+              result.SetLink('%s_FAIL' % failure_filename, link)

[jbudorick, 2017/02/21 18:32:39]

nit: name the link something else. Not immediately sure what, though.

[mikecase (-- gone --), 2017/02/23 00:21:26]

I changed this since I didnt want three separate links for _FAIL, _GOLDEN, and
now _DIFF

Instead I embed the three images into a very simple HTML page and only link
that.

+
+            golden_filepath = os.path.join(
+                host_paths.DIR_SOURCE_ROOT, render_results_dir,
+                failure_filename)
+            if not os.path.exists(golden_filepath):
+              logging.error('Cannot find golden image for %s', failure_filename)
+              continue
+
+            link = logdog_helper.image(
+                logdog_helper.unique_name(failure_filename, device=device),
+                golden_filepath)
+            for result in results:
+              result.SetLink('%s_GOLDEN' % failure_filename, link)

[jbudorick, 2017/02/21 18:32:39]

nit: same

[mikecase (-- gone --), 2017/02/23 00:21:26]

See above.

+
+        finally:
+          if temp_dir:
+            shutil.rmtree(temp_dir)
+
     # Update the result name if the test used flags.
     if flags:
       for r in results:
@@ -326,11 +388,18 @@ class LocalDeviceInstrumentationTestRun(
         file_name = '%s-%s.png' % (
             test_display_name,
             time.strftime('%Y%m%dT%H%M%S', time.localtime()))
-        saved_dir = device.TakeScreenshot(
+        screenshot_file = device.TakeScreenshot(
             os.path.join(self._test_instance.screenshot_dir, file_name))
         logging.info(
             'Saved screenshot for %s to %s.',
-            test_display_name, saved_dir)
+            test_display_name, screenshot_file)
+        if self._test_instance.should_save_images:
+          link = logdog_helper.image(
+              logdog_helper.unique_name('screenshot', device=device),
+              screenshot_file)
+          for result in results:
+            result.SetLink('failure_screenshot', link)
+
       logging.info('detected failure in %s. raw output:', test_display_name)
       for l in output:
         logging.info('  %s', l)
@@ -342,7 +411,6 @@ class LocalDeviceInstrumentationTestRun(
             else None)
         device.ClearApplicationState(self._test_instance.package_info.package,
                                      permissions=permissions)
-
     else:
       logging.debug('raw output from %s:', test_display_name)
       for l in output: