(Reland) Add failure screenshots and images to results detail.

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 tempfile
 import time
 
 from devil.android import device_errors
 from devil.android import flag_changer
 from devil.android.sdk import shared_prefs
 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
 from pylib.local.device import local_device_test_run
+from pylib.utils import google_storage_helper
 from pylib.utils import logdog_helper
 from py_trace_event import trace_event
 from py_utils import contextlib_ext
+from py_utils import tempfile_ext
 import tombstones
 
+try:
+  from PIL import Image  # pylint: disable=import-error
+  from PIL import ImageChops  # pylint: disable=import-error
+  can_compute_diffs = True
+except ImportError:
+  can_compute_diffs = False
+
 _TAG = 'test_runner_py'
 
 TIMEOUT_ANNOTATIONS = [
   ('Manual', 10 * 60 * 60),
   ('IntegrationTest', 30 * 60),
   ('External', 10 * 60),
   ('EnormousTest', 10 * 60),
   ('LargeTest', 5 * 60),
   ('MediumTest', 3 * 60),
   ('SmallTest', 1 * 60),
 ]
 
 LOGCAT_FILTERS = ['*:e', 'chromium:v', 'cr_*:v']
 
+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 = {
+  'ChromePublicTest': 'chrome/test/data/android/render_tests'

[jbudorick, 2017/04/29 01:12:26]

Can we have the APK store this information in the manifest rather than having it
hard-coded here?

[mikecase (-- gone --), 2017/05/03 21:25:14]

I think this is a great idea. Unfortunately, I cant think of a clean way to do
it. I mean, we could pass the render_dir as an EXTRA or something to the
instrumentation test? That might work. 

For now, will change it to a map from test_package_name: render_dir and add a
big TODO.

+}
+
 # TODO(jbudorick): Make this private once the instrumentation test_runner is
 # deprecated.
 def DidPackageCrashOnDevice(package_name, device):
   # Dismiss any error dialogs. Limit the number in case we have an error
   # loop or we are failing to dismiss.
   try:
     for _ in xrange(10):
       package = device.DismissCrashDialogIfNeeded()
       if not package:
         return False
@@ skipping 293 matching lines @@
     # TODO(jbudorick): Make instrumentation tests output a JSON so this
     # doesn't have to parse the output.
     result_code, result_bundle, statuses = (
         self._test_instance.ParseAmInstrumentRawOutput(output))
     results = self._test_instance.GenerateTestResults(
         result_code, result_bundle, statuses, start_ms, duration_ms)
     for result in results:
       if logcat_url:
         result.SetLink('logcat', logcat_url)
 
+    self._ProcessRenderTestResults(device, results)
+
     # Update the result name if the test used flags.
     if flags:
       for r in results:
         if r.GetName() == test_name:
           r.SetName(test_display_name)
 
     # Add UNKNOWN results for any missing tests.
     iterable_test = test if isinstance(test, list) else [test]
     test_names = set(self._GetUniqueTestName(t) for t in iterable_test)
     results_names = set(r.GetName() for r in results)
@@ skipping 11 matching lines @@
     #   - optionally taking a screenshot
     #   - logging the raw output at INFO level
     #   - 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):
       if self._test_instance.screenshot_dir:
         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:

[jbudorick, 2017/04/29 01:12:26]

We only get here if screenshot_dir is also set. Do we want to require that?
Could we save to a temporary directory if we want to save images but we don't
have a screenshot directory configured?

[mikecase (-- gone --), 2017/05/03 21:25:14]

I had a CL to do that I uploaded after the first time this landed....
https://codereview.chromium.org/2721323003

Will close that CL and merge it into this one.

+          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('failure_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)
         device.ClearApplicationState(self._test_instance.package_info.package,
                                      permissions=permissions)
-
     else:
       logging.debug('raw output from %s:', test_display_name)
       for l in output:
         logging.debug('  %s', l)
     if self._test_instance.coverage_directory:
       device.PullFile(coverage_directory,
           self._test_instance.coverage_directory)
       device.RunShellCommand(
           'rm -f %s' % posixpath.join(coverage_directory, '*'),
           check_return=True, shell=True)
     if self._test_instance.store_tombstones:
       tombstones_url = None
       for result in results:
         if result.GetType() == base_test_result.ResultType.CRASH:
           if not tombstones_url:
             resolved_tombstones = tombstones.ResolveTombstones(
                 device,
                 resolve_all_tombstones=True,
                 include_stack_symbols=False,
                 wipe_tombstones=True)
             stream_name = 'tombstones_%s_%s' % (
                 time.strftime('%Y%m%dT%H%M%S', time.localtime()),
                 device.serial)
             tombstones_url = logdog_helper.text(
                 stream_name, '\n'.join(resolved_tombstones))
           result.SetLink('tombstones', tombstones_url)
     return results, None
 
+  def _ProcessRenderTestResults(self, device, results):
+    if not self._test_instance.should_save_images:
+      return
+
+    render_results_dir = RENDER_TESTS_RESULTS_DIR.get(self._test_instance.suite)
+    if not render_results_dir:
+      return
+
+    failure_images_device_dir = posixpath.join(
+        device.GetExternalStoragePath(),
+        'chromium_tests_root', render_results_dir, 'failures')
+    if not device.FileExists(failure_images_device_dir):
+      return
+
+    with tempfile_ext.NamedTemporaryDirectory() as temp_dir:
+      device.PullFile(failure_images_device_dir, temp_dir)
+      device.RemovePath(failure_images_device_dir, recursive=True)
+
+      for failure_filename in os.listdir(os.path.join(temp_dir, 'failures')):
+
+        m = RE_RENDER_IMAGE_NAME.match(failure_filename)
+        if not m:
+          logging.warning('Unexpected file in render test failures: %s',
+                          failure_filename)
+          continue
+
+        failure_filepath = os.path.join(temp_dir, 'failures', failure_filename)
+        failure_link = google_storage_helper.upload(
+            google_storage_helper.unique_name(failure_filename, device=device),
+            failure_filepath,
+            bucket=self._test_instance.gs_results_bucket + '/render_tests')

[jbudorick, 2017/04/29 01:12:26]

You've got this in here four times. Should probably have a common way of dealing
with it.

[mikecase (-- gone --), 2017/05/03 21:25:14]

Fixed. But going to move render stuff stuff to new CL

+
+        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
+        golden_link = google_storage_helper.upload(
+            google_storage_helper.unique_name(failure_filename, device=device),
+            golden_filepath,
+            bucket=self._test_instance.gs_results_bucket + '/render_tests')
+
+        if can_compute_diffs:
+          diff_filename = '_diff'.join(os.path.splitext(failure_filename))
+          diff_filepath = os.path.join(temp_dir, diff_filename)
+          (ImageChops.difference(

[jbudorick, 2017/04/29 01:12:26]

We're already comparing the images in the APK. Should we be computing the diff
there, too?

[mikecase (-- gone --), 2017/05/03 21:25:14]

Changing to Java side diffing. Will make CL a bit bigger so going to split it
up. Render stuff is going in new CL.

+              Image.open(failure_filepath), Image.open(golden_filepath))
+                  .convert('L')
+                  .point(lambda i: 255 if i else 0)
+                  .save(diff_filepath))
+          diff_link = google_storage_helper.upload(
+              google_storage_helper.unique_name(diff_filename, device=device),
+              diff_filepath,
+              bucket=self._test_instance.gs_results_bucket + '/render_tests')
+        else:
+          diff_link = ''
+          logging.error('Error importing PIL library. Image diffs for '
+                        'render test results will not be computed.')
+
+        with tempfile.NamedTemporaryFile(suffix='.html') as temp_html:
+          temp_html.write('''

[jbudorick, 2017/04/29 01:12:26]

nit: pull this into a constant at module scope.

[mikecase (-- gone --), 2017/05/03 21:25:14]

Done.

+              <html>
+              <table>
+              <tr>
+                <th>Failure</th>
+                <th>Golden</th>
+                <th>Diff</th>
+              </tr>
+              <tr>
+                <td><img src="%s"/></td>
+                <td><img src="%s"/></td>
+                <td><img src="%s"/></td>
+              </tr>
+              </table>
+              </html>
+              ''' % (failure_link, golden_link, diff_link))
+          temp_html.flush()
+          html_results_link = google_storage_helper.upload(
+              google_storage_helper.unique_name('render_html', device=device),
+              temp_html.name,
+              bucket=self._test_instance.gs_results_bucket + '/render_tests',
+              content_type='text/html')
+          for result in results:
+            result.SetLink(failure_filename, html_results_link)
+
   #override
   def _ShouldRetry(self, test):
     if 'RetryOnFailure' in test.get('annotations', {}):
       return True
 
     # TODO(jbudorick): Remove this log message once @RetryOnFailure has been
     # enabled for a while. See crbug.com/619055 for more details.
     logging.error('Default retries are being phased out. crbug.com/619055')
     return False
 
@@ skipping 15 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