Deduplicate Android/GPU Gold code

Deduplicates the Skia Gold code used by the Android instrumentation
tests and the GPU Telemetry-based pixel tests, as they were effectively
identical with only a few differences. This is accomplished by moving
the majority of the code to //testing/skia_gold_common and having each
test harness extend the abstract classes as necessary.

Bug: 1093994
Change-Id: Icef9f22d896a82672f3c64a26d38c026bf6bd02d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253147Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: John Chen <johnchen@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781137}

build/android/PRESUBMIT.py

@@ -21,6 +21,8 @@ def CommonChecks(input_api, output_api):
       r'gyp/.*\.py$',
   ]
   tests = []
+  # yapf likes formatting the extra_paths_list to be less readable.
+  # yapf: disable
   tests.extend(
       input_api.canned_checks.GetPylint(
           input_api,
@@ -41,6 +43,7 @@ def CommonChecks(input_api, output_api):
               J('..', '..', 'third_party', 'catapult', 'tracing'),
               J('..', '..', 'third_party', 'depot_tools'),
               J('..', '..', 'third_party', 'colorama', 'src'),
+              J('..', '..', 'testing'),
           ]))
   tests.extend(
       input_api.canned_checks.GetPylint(
@@ -51,6 +54,7 @@ def CommonChecks(input_api, output_api):
               r'.*_pb2\.py',
           ],
           extra_paths_list=[J('gyp'), J('gn')]))
+  # yapf: enable
 
   # Disabled due to http://crbug.com/410936
   #output.extend(input_api.canned_checks.RunUnitTestsInDirectory(

build/android/pylib/constants/host_paths.py

@@ -22,6 +22,7 @@ ANDROID_PLATFORM_DEVELOPMENT_SCRIPTS_PATH = os.path.join(
     'scripts')
 DEVIL_PATH = os.path.join(
     DIR_SOURCE_ROOT, 'third_party', 'catapult', 'devil')
+TESTING_PATH = os.path.join(DIR_SOURCE_ROOT, 'testing')
 TRACING_PATH = os.path.join(
     DIR_SOURCE_ROOT, 'third_party', 'catapult', 'tracing')
 

build/android/pylib/instrumentation/instrumentation_test_instance.py

@@ -760,7 +760,7 @@ class InstrumentationTestInstance(test_instance.TestInstance):
     self._use_webview_provider = args.use_webview_provider
 
   def _initializeSkiaGoldAttributes(self, args):
-    self._skia_gold_properties = gold_utils.SkiaGoldProperties(args)
+    self._skia_gold_properties = gold_utils.AndroidSkiaGoldProperties(args)
 
   @property
   def additional_apks(self):

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

@@ -388,7 +388,7 @@ class LocalDeviceInstrumentationTestRun(
     # expectations can be re-used between tests, saving a significant amount
     # of time.
     self._skia_gold_work_dir = tempfile.mkdtemp()
-    self._skia_gold_session_manager = gold_utils.SkiaGoldSessionManager(
+    self._skia_gold_session_manager = gold_utils.AndroidSkiaGoldSessionManager(
         self._skia_gold_work_dir, self._test_instance.skia_gold_properties)
     if self._test_instance.wait_for_java_debugger:
       logging.warning('*' * 80)
@@ -1031,7 +1031,7 @@ class LocalDeviceInstrumentationTestRun(
             json.dump(json_dict, outfile)
 
         gold_session = self._skia_gold_session_manager.GetSkiaGoldSession(
-            keys_file=json_path)
+            keys_input=json_path)
 
         try:
           status, error = gold_session.RunComparison(
@@ -1069,7 +1069,7 @@ class LocalDeviceInstrumentationTestRun(
         failure_log = (
             'Skia Gold reported failure for RenderTest %s. See '
             'RENDER_TESTS.md for how to fix this failure.' % render_name)
-        status_codes = gold_utils.SkiaGoldSession.StatusCodes
+        status_codes = gold_utils.AndroidSkiaGoldSession.StatusCodes
         if status == status_codes.AUTH_FAILURE:
           _AppendToLog(results,
                        'Gold authentication failed with output %s' % error)

build/android/test_runner.pydeps

 # Generated by running:
 #   build/print_python_deps.py --root build/android --output build/android/test_runner.pydeps build/android/test_runner.py
+../../testing/skia_gold_common/__init__.py
+../../testing/skia_gold_common/skia_gold_properties.py
+../../testing/skia_gold_common/skia_gold_session.py
+../../testing/skia_gold_common/skia_gold_session_manager.py
 ../../third_party/catapult/common/py_trace_event/py_trace_event/__init__.py
 ../../third_party/catapult/common/py_trace_event/py_trace_event/trace_event.py
 ../../third_party/catapult/common/py_trace_event/py_trace_event/trace_event_impl/__init__.py

chrome/test/BUILD.gn

@@ -6806,6 +6806,9 @@ if (!is_fuchsia && !is_android) {
       "//testing/scripts/common.py",
       "//testing/xvfb.py",
       "//testing/scripts/run_telemetry_as_googletest.py",
+      
+      # For Skia Gold code, which has some GPU-specific unittests.
+      "//testing/skia_gold_common/",
     ]
   }
 }

content/test/BUILD.gn

@@ -658,6 +658,9 @@ group("telemetry_gpu_integration_test_support") {
 
     # For power
     "//media/test/data/bear-1280x720.mp4",
+
+    # For Skia Gold pixel test functionality.
+    "//testing/skia_gold_common/",
   ]
 
   data_deps = [

content/test/gpu/gpu_tests/skia_gold/gpu_skia_gold_properties.py

+# Copyright 2020 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.
+"""GPU implementation of //testing/skia_gold_common/skia_gold_properties.py."""
+
+import subprocess
+import sys
+
+from gpu_tests import path_util
+path_util.AddDirToPathIfNeeded(path_util.GetChromiumSrcDir(), 'testing')
+from skia_gold_common import skia_gold_properties
+
+
+class GpuSkiaGoldProperties(skia_gold_properties.SkiaGoldProperties):
+  @staticmethod
+  def _GetGitOriginMasterHeadSha1():
+    try:
+      return subprocess.check_output(['git', 'rev-parse', 'origin/master'],
+                                     shell=_IsWin(),
+                                     cwd=path_util.GetChromiumSrcDir()).strip()
+    except subprocess.CalledProcessError:
+      return None
+
+
+def _IsWin():
+  return sys.platform == 'win32'

content/test/gpu/gpu_tests/skia_gold/gpu_skia_gold_session.py

+# Copyright 2020 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.
+"""GPU implementation of //testing/skia_gold_common/skia_gold_session.py."""
+
+import os
+import subprocess
+import tempfile
+
+from gpu_tests import path_util
+path_util.AddDirToPathIfNeeded(path_util.GetChromiumSrcDir(), 'testing')
+from skia_gold_common import skia_gold_session
+
+
+class GpuSkiaGoldSession(skia_gold_session.SkiaGoldSession):
+  def RunComparison(self, name, png_file, output_manager=True, use_luci=True):
+    # Passing True for the output manager is a bit of a hack, as we don't
+    # actually need an output manager and just need to get past the truthy
+    # check.
+    return super(GpuSkiaGoldSession,
+                 self).RunComparison(name=name,
+                                     png_file=png_file,
+                                     output_manager=output_manager,
+                                     use_luci=use_luci)
+
+  def _CreateDiffOutputDir(self):
+    # We intentionally don't clean this up and don't put it in self._working_dir
+    # since we need it to stick around after the test completes so the user
+    # can look at its contents.
+    return tempfile.mkdtemp()
+
+  def _StoreDiffLinks(self, image_name, _, output_dir):
+    results = self._comparison_results.setdefault(image_name,
+                                                  self.ComparisonResults())
+    # The directory should contain "input-<hash>.png", "closest-<hash>.png",
+    # and "diff.png".
+    for f in os.listdir(output_dir):
+      file_url = 'file://%s' % os.path.join(output_dir, f)
+      if f.startswith('input-'):
+        results.local_diff_given_image = file_url
+      elif f.startswith('closest-'):
+        results.local_diff_closest_image = file_url
+      elif f == 'diff.png':
+        results.local_diff_diff_image = file_url
+
+  @staticmethod
+  def _RunCmdForRcAndOutput(cmd):
+    try:
+      output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+      return 0, output
+    except subprocess.CalledProcessError as e:
+      return e.returncode, e.output

content/test/gpu/gpu_tests/skia_gold/gpu_skia_gold_session_manager.py

+# Copyright 2020 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.
+"""GPU impl of //testing/skia_gold_common/skia_gold_session_manager.py."""
+
+from gpu_tests import path_util
+from gpu_tests.skia_gold import gpu_skia_gold_session
+
+path_util.AddDirToPathIfNeeded(path_util.GetChromiumSrcDir(), 'testing')
+from skia_gold_common import skia_gold_session_manager as sgsm
+
+
+class GpuSkiaGoldSessionManager(sgsm.SkiaGoldSessionManager):
+  @staticmethod
+  def _GetDefaultInstance():
+    return 'chrome-gpu'
+
+  @staticmethod
+  def _GetSessionClass():
+    return gpu_skia_gold_session.GpuSkiaGoldSession

content/test/gpu/gpu_tests/skia_gold/gpu_skia_gold_session_unittest.py

+#!/usr/bin/env vpython
+# Copyright 2020 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.
+
+#pylint: disable=protected-access
+
+import os
+import tempfile
+import unittest
+
+import mock
+
+from gpu_tests import path_util
+from gpu_tests.skia_gold import gpu_skia_gold_properties
+from gpu_tests.skia_gold import gpu_skia_gold_session
+
+from pyfakefs import fake_filesystem_unittest
+
+path_util.AddDirToPathIfNeeded(path_util.GetChromiumSrcDir(), 'testing')
+from skia_gold_common import unittest_utils
+
+createSkiaGoldArgs = unittest_utils.createSkiaGoldArgs
+
+
+def assertArgWith(test, arg_list, arg, value):
+  i = arg_list.index(arg)
+  test.assertEqual(arg_list[i + 1], value)
+
+
+class GpuSkiaGoldSessionDiffTest(fake_filesystem_unittest.TestCase):
+  def setUp(self):
+    self.setUpPyfakefs()
+    self._working_dir = tempfile.mkdtemp()
+
+  @mock.patch.object(gpu_skia_gold_session.GpuSkiaGoldSession,
+                     '_RunCmdForRcAndOutput')
+  def test_commandCommonArgs(self, cmd_mock):
+    cmd_mock.return_value = (None, None)
+    args = createSkiaGoldArgs(git_revision='a', local_pixel_tests=False)
+    sgp = gpu_skia_gold_properties.GpuSkiaGoldProperties(args)
+    session = gpu_skia_gold_session.GpuSkiaGoldSession(self._working_dir,
+                                                       sgp,
+                                                       None,
+                                                       'corpus',
+                                                       instance='instance')
+    session.Diff('name', 'png_file', None)
+    call_args = cmd_mock.call_args[0][0]
+    self.assertIn('diff', call_args)
+    assertArgWith(self, call_args, '--corpus', 'corpus')
+    assertArgWith(self, call_args, '--instance', 'instance')
+    assertArgWith(self, call_args, '--input', 'png_file')
+    assertArgWith(self, call_args, '--test', 'name')
+    assertArgWith(self, call_args, '--work-dir', self._working_dir)
+    i = call_args.index('--out-dir')
+    # The output directory should not be a subdirectory of the working
+    # directory.
+    self.assertNotIn(self._working_dir, call_args[i + 1])
+
+
+class GpuSkiaGoldSessionStoreDiffLinksTest(fake_filesystem_unittest.TestCase):
+  def setUp(self):
+    self.setUpPyfakefs()
+    self._working_dir = tempfile.mkdtemp()
+
+  def test_outputManagerNotNeeded(self):
+    args = createSkiaGoldArgs(git_revision='a', local_pixel_tests=True)
+    sgp = gpu_skia_gold_properties.GpuSkiaGoldProperties(args)
+    session = gpu_skia_gold_session.GpuSkiaGoldSession(self._working_dir, sgp,
+                                                       None, None, None)
+    input_filepath = os.path.join(self._working_dir, 'input-inputhash.png')
+    with open(input_filepath, 'w') as f:
+      f.write('')
+    closest_filepath = os.path.join(self._working_dir,
+                                    'closest-closesthash.png')
+    with open(closest_filepath, 'w') as f:
+      f.write('')
+    diff_filepath = os.path.join(self._working_dir, 'diff.png')
+    with open(diff_filepath, 'w') as f:
+      f.write('')
+
+    session._StoreDiffLinks('foo', None, self._working_dir)
+    self.assertEqual(session.GetGivenImageLink('foo'),
+                     'file://' + input_filepath)
+    self.assertEqual(session.GetClosestImageLink('foo'),
+                     'file://' + closest_filepath)
+    self.assertEqual(session.GetDiffImageLink('foo'), 'file://' + diff_filepath)
+
+
+if __name__ == '__main__':
+  unittest.main(verbosity=2)

content/test/gpu/gpu_tests/skia_gold_integration_test_base.py

@@ -13,9 +13,9 @@ import tempfile
 from gpu_tests import gpu_integration_test
 from gpu_tests import path_util
 from gpu_tests import color_profile_manager
-from gpu_tests.skia_gold import skia_gold_properties
-from gpu_tests.skia_gold import skia_gold_session
-from gpu_tests.skia_gold import skia_gold_session_manager
+from gpu_tests.skia_gold import gpu_skia_gold_properties
+from gpu_tests.skia_gold import gpu_skia_gold_session
+from gpu_tests.skia_gold import gpu_skia_gold_session_manager
 
 from py_utils import cloud_storage
 
@@ -87,15 +87,16 @@ class SkiaGoldIntegrationTestBase(gpu_integration_test.GpuIntegrationTest):
   @classmethod
   def GetSkiaGoldProperties(cls):
     if not cls._skia_gold_properties:
-      cls._skia_gold_properties = skia_gold_properties.SkiaGoldProperties(
-          cls.GetParsedCommandLineOptions())
+      cls._skia_gold_properties =\
+          gpu_skia_gold_properties.GpuSkiaGoldProperties(
+              cls.GetParsedCommandLineOptions())
     return cls._skia_gold_properties
 
   @classmethod
   def GetSkiaGoldSessionManager(cls):
     if not cls._skia_gold_session_manager:
       cls._skia_gold_session_manager =\
-          skia_gold_session_manager.SkiaGoldSessionManager(
+          gpu_skia_gold_session_manager.GpuSkiaGoldSessionManager(
               cls._skia_gold_temp_dir, cls.GetSkiaGoldProperties())
     return cls._skia_gold_session_manager
 
@@ -419,7 +420,7 @@ class SkiaGoldIntegrationTestBase(gpu_integration_test.GpuIntegrationTest):
     if not status:
       return
 
-    status_codes = skia_gold_session.SkiaGoldSession.StatusCodes
+    status_codes = gpu_skia_gold_session.GpuSkiaGoldSession.StatusCodes
     if status == status_codes.AUTH_FAILURE:
       logging.error('Gold authentication failed with output %s', error)
     elif status == status_codes.INIT_FAILURE:

testing/PRESUBMIT.py

@@ -14,6 +14,14 @@ def CommonChecks(input_api, output_api):
   blacklist = [r'gmock.*', r'gtest.*']
   output.extend(input_api.canned_checks.RunUnitTestsInDirectory(
       input_api, output_api, '.', [r'^.+_unittest\.py$']))
+  skia_gold_env = dict(input_api.environ)
+  skia_gold_env.update({
+      'PYTHONPATH': input_api.PresubmitLocalPath(),
+      'PYTHONDONTWRITEBYTECODE': '1',
+  })
+  output.extend(input_api.canned_checks.RunUnitTestsInDirectory(
+      input_api, output_api, 'skia_gold_common', [r'^.+_unittest\.py$'],
+      env=skia_gold_env))
   output.extend(input_api.canned_checks.RunPylint(
       input_api, output_api, black_list=blacklist))
   return output

testing/skia_gold_common/.style.yapf

+[style]
+based_on_style = pep8
+
+column_limit = 80
+indent_width = 2

testing/skia_gold_common/OWNERS

+bsheedy@google.com

testing/skia_gold_common/__init__.py

+# Copyright 2020 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.

content/test/gpu/gpu_tests/skia_gold/skia_gold_properties.py → testing/skia_gold_common/skia_gold_properties.py

@@ -14,12 +14,10 @@ import os
 import subprocess
 import sys
 
-from gpu_tests import path_util
-
 
 class SkiaGoldProperties(object):
   def __init__(self, args):
-    """Class to validate and store properties related to Skia Gold.
+    """Abstract class to validate and store properties related to Skia Gold.
 
     Args:
       args: The parsed arguments from an argparse.ArgumentParser.
@@ -78,6 +76,10 @@ class SkiaGoldProperties(object):
   def bypass_skia_gold_functionality(self):
     return self._bypass_skia_gold_functionality
 
+  @staticmethod
+  def _GetGitOriginMasterHeadSha1():
+    raise NotImplementedError()
+
   def _GetGitRevision(self):
     if not self._git_revision:
       # Automated tests should always pass the revision, so assume we're on
@@ -85,7 +87,7 @@ class SkiaGoldProperties(object):
       if not self._IsLocalRun():
         raise RuntimeError(
             '--git-revision was not passed when running on a bot')
-      revision = _GetGitOriginMasterHeadSha1()
+      revision = self._GetGitOriginMasterHeadSha1()
       if not revision or len(revision) != 40:
         raise RuntimeError(
             '--git-revision not passed and unable to determine from git')
@@ -135,16 +137,3 @@ class SkiaGoldProperties(object):
       raise RuntimeError(
           '--gerrit-issue passed, but --buildbucket-id not passed.')
     self._job_id = args.buildbucket_id
-
-
-def _IsWin():
-  return sys.platform == 'win32'
-
-
-def _GetGitOriginMasterHeadSha1():
-  try:
-    return subprocess.check_output(['git', 'rev-parse', 'origin/master'],
-                                   shell=_IsWin(),
-                                   cwd=path_util.GetChromiumSrcDir()).strip()
-  except subprocess.CalledProcessError:
-    return None

content/test/gpu/gpu_tests/skia_gold/skia_gold_properties_unittest.py → testing/skia_gold_common/skia_gold_properties_unittest.py

@@ -10,8 +10,8 @@ import unittest
 
 import mock
 
-from gpu_tests.skia_gold import skia_gold_properties
-from gpu_tests.skia_gold import unittest_utils
+from skia_gold_common import skia_gold_properties
+from skia_gold_common import unittest_utils
 
 createSkiaGoldArgs = unittest_utils.createSkiaGoldArgs
 
@@ -132,9 +132,8 @@ class SkiaGoldPropertiesCalculationTest(unittest.TestCase):
   def testGetGitRevision_findValidRevision(self):
     args = createSkiaGoldArgs(local_pixel_tests=True)
     sgp = skia_gold_properties.SkiaGoldProperties(args)
-    with mock.patch(
-        'gpu_tests.skia_gold.skia_gold_properties._GetGitOriginMasterHeadSha1'
-    ) as patched_head:
+    with mock.patch.object(skia_gold_properties.SkiaGoldProperties,
+                           '_GetGitOriginMasterHeadSha1') as patched_head:
       expected = 'a' * 40
       patched_head.return_value = expected
       self.assertEqual(sgp.git_revision, expected)
@@ -150,9 +149,8 @@ class SkiaGoldPropertiesCalculationTest(unittest.TestCase):
   def testGetGitRevision_findEmptyRevision(self):
     args = createSkiaGoldArgs(local_pixel_tests=True)
     sgp = skia_gold_properties.SkiaGoldProperties(args)
-    with mock.patch(
-        'gpu_tests.skia_gold.skia_gold_properties._GetGitOriginMasterHeadSha1'
-    ) as patched_head:
+    with mock.patch.object(skia_gold_properties.SkiaGoldProperties,
+                           '_GetGitOriginMasterHeadSha1') as patched_head:
       patched_head.return_value = ''
       with self.assertRaises(RuntimeError):
         _ = sgp.git_revision
@@ -160,9 +158,8 @@ class SkiaGoldPropertiesCalculationTest(unittest.TestCase):
   def testGetGitRevision_findMalformedRevision(self):
     args = createSkiaGoldArgs(local_pixel_tests=True)
     sgp = skia_gold_properties.SkiaGoldProperties(args)
-    with mock.patch(
-        'gpu_tests.skia_gold.skia_gold_properties._GetGitOriginMasterHeadSha1'
-    ) as patched_head:
+    with mock.patch.object(skia_gold_properties.SkiaGoldProperties,
+                           '_GetGitOriginMasterHeadSha1') as patched_head:
       patched_head.return_value = 'a' * 39
       with self.assertRaises(RuntimeError):
         _ = sgp.git_revision

content/test/gpu/gpu_tests/skia_gold/skia_gold_session.py → testing/skia_gold_common/skia_gold_session.py

 # Copyright 2020 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.
-"""Class for interacting with the Skia Gold image diffing service.
-
-This is based heavily off Android's Skia Gold implementation in
-//build/android/pylib/utils/gold_utils.py. If you need to make a change to this
-file, check to see if the same change needs to be made there.
-"""
+"""Class for interacting with the Skia Gold image diffing service."""
 
 import logging
 import os
@@ -14,10 +9,10 @@ import subprocess
 import sys
 import tempfile
 
-from gpu_tests import path_util
+CHROMIUM_SRC = os.path.realpath(
+    os.path.join(os.path.dirname(__file__), '..', '..'))
 
-GOLDCTL_BINARY = os.path.join(path_util.GetChromiumSrcDir(), 'tools',
-                              'skia_goldctl')
+GOLDCTL_BINARY = os.path.join(CHROMIUM_SRC, 'tools', 'skia_goldctl')
 if sys.platform == 'win32':
   GOLDCTL_BINARY = os.path.join(GOLDCTL_BINARY, 'win', 'goldctl') + '.exe'
 elif sys.platform == 'darwin':
@@ -35,6 +30,7 @@ class SkiaGoldSession(object):
     COMPARISON_FAILURE_REMOTE = 3
     COMPARISON_FAILURE_LOCAL = 4
     LOCAL_DIFF_FAILURE = 5
+    NO_OUTPUT_MANAGER = 6
 
   class ComparisonResults(object):
     """Struct-like object for storing results of an image comparison."""
@@ -47,7 +43,7 @@ class SkiaGoldSession(object):
       self.local_diff_diff_image = None
 
   def __init__(self, working_dir, gold_properties, keys_file, corpus, instance):
-    """A class to handle all aspects of an image comparison via Skia Gold.
+    """Abstract class to handle all aspects of image comparison via Skia Gold.
 
     A single SkiaGoldSession is valid for a single instance/corpus/keys_file
     combination.
@@ -75,7 +71,7 @@ class SkiaGoldSession(object):
     self._authenticated = False
     self._initialized = False
 
-  def RunComparison(self, name, png_file, use_luci=True):
+  def RunComparison(self, name, png_file, output_manager, use_luci=True):
     """Helper method to run all steps to compare a produced image.
 
     Handles authentication, itnitialization, comparison, and, if necessary,
@@ -84,6 +80,10 @@ class SkiaGoldSession(object):
     Args:
       name: The name of the image being compared.
       png_file: A path to a PNG file containing the image to be compared.
+       output_manager: An output manager to use to store diff links. The
+          argument's type depends on what type a subclasses' _StoreDiffLinks
+          implementation expects. Can be None even if _StoreDiffLinks expects
+          a valid input, but will fail if it ever actually needs to be used.
       use_luci: If true, authentication will use the service account provided by
           the LUCI context. If false, will attempt to use whatever is set up in
           gsutil, which is only supported for local runs.
@@ -109,7 +109,13 @@ class SkiaGoldSession(object):
     if not self._gold_properties.local_pixel_tests:
       return self.StatusCodes.COMPARISON_FAILURE_REMOTE, compare_stdout
 
-    diff_rc, diff_stdout = self.Diff(name=name, png_file=png_file)
+    if not output_manager:
+      return (self.StatusCodes.NO_OUTPUT_MANAGER,
+              'No output manager for local diff images')
+
+    diff_rc, diff_stdout = self.Diff(name=name,
+                                     png_file=png_file,
+                                     output_manager=output_manager)
     if diff_rc:
       return self.StatusCodes.LOCAL_DIFF_FAILURE, diff_stdout
     return self.StatusCodes.COMPARISON_FAILURE_LOCAL, compare_stdout
@@ -142,7 +148,7 @@ class SkiaGoldSession(object):
           'Cannot authenticate to Skia Gold with use_luci=False unless running '
           'local pixel tests')
 
-    rc, stdout = _RunCmdForRcAndOutput(auth_cmd)
+    rc, stdout = self._RunCmdForRcAndOutput(auth_cmd)
     if rc == 0:
       self._authenticated = True
     return rc, stdout
@@ -199,7 +205,7 @@ class SkiaGoldSession(object):
           str(self._gold_properties.continuous_integration_system),
       ])
 
-    rc, stdout = _RunCmdForRcAndOutput(init_cmd)
+    rc, stdout = self._RunCmdForRcAndOutput(init_cmd)
     if rc == 0:
       self._initialized = True
     return rc, stdout
@@ -238,17 +244,18 @@ class SkiaGoldSession(object):
       compare_cmd.append('--dryrun')
 
     self._ClearTriageLinkFile()
-    rc, stdout = _RunCmdForRcAndOutput(compare_cmd)
+    rc, stdout = self._RunCmdForRcAndOutput(compare_cmd)
 
     self._comparison_results[name] = self.ComparisonResults()
     if rc == 0:
       self._comparison_results[name].triage_link_omission_reason = (
           'Comparison succeeded, no triage link')
     elif self._gold_properties.IsTryjobRun():
-      cl_triage_link = ('https://{instance}-gold.skia.org/search?'
-                        'issue={issue}')
-      cl_triage_link = cl_triage_link.format(instance=self._instance,
-                                             issue=self._gold_properties.issue)
+      cl_triage_link = ('https://{instance}-gold.skia.org/cl/{crs}/{issue}')
+      cl_triage_link = cl_triage_link.format(
+          instance=self._instance,
+          crs=self._gold_properties.code_review_system,
+          issue=self._gold_properties.issue)
       self._comparison_results[name].triage_link = cl_triage_link
     else:
       try:
@@ -260,7 +267,7 @@ class SkiaGoldSession(object):
             'Failed to read triage link from file')
     return rc, stdout
 
-  def Diff(self, name, png_file):
+  def Diff(self, name, png_file, output_manager):
     """Performs a local image diff against the closest known positive in Gold.
 
     This is used for running tests on a workstation, where uploading data to
@@ -271,6 +278,9 @@ class SkiaGoldSession(object):
     Args:
       name: The name of the image being compared.
       png_file: The path to a PNG file containing the image to be diffed.
+      output_manager: An output manager to use to store diff links. The
+          argument's type depends on what type a subclasses' _StoreDiffLinks
+          implementation expects.
 
     Returns:
       A tuple (return_code, output). |return_code| is the return code of the
@@ -284,10 +294,7 @@ class SkiaGoldSession(object):
           '--bypass-skia-gold-functionality is not supported when running '
           'tests locally.')
 
-    # We intentionally don't clean this up and don't put it in self._working_dir
-    # since we need it to stick around after the test completes so the user
-    # can look at its contents.
-    output_dir = tempfile.mkdtemp()
+    output_dir = self._CreateDiffOutputDir()
     diff_cmd = [
         GOLDCTL_BINARY,
         'diff',
@@ -304,19 +311,8 @@ class SkiaGoldSession(object):
         '--out-dir',
         output_dir,
     ]
-    rc, stdout = _RunCmdForRcAndOutput(diff_cmd)
-    results = self._comparison_results.setdefault(name,
-                                                  self.ComparisonResults())
-    # The directory should contain "input-<hash>.png", "closest-<hash>.png",
-    # and "diff.png".
-    for f in os.listdir(output_dir):
-      file_url = 'file://%s' % os.path.join(output_dir, f)
-      if f.startswith('input-'):
-        results.local_diff_given_image = file_url
-      elif f.startswith('closest-'):
-        results.local_diff_closest_image = file_url
-      elif f == 'diff.png':
-        results.local_diff_diff_image = file_url
+    rc, stdout = self._RunCmdForRcAndOutput(diff_cmd)
+    self._StoreDiffLinks(name, output_manager, output_dir)
     return rc, stdout
 
   def GetTriageLink(self, name):
@@ -403,10 +399,34 @@ class SkiaGoldSession(object):
     """
     open(self._triage_link_file, 'w').close()
 
+  def _CreateDiffOutputDir(self):
+    return tempfile.mkdtemp(dir=self._working_dir)
+
+  def _StoreDiffLinks(self, image_name, output_manager, output_dir):
+    """Stores the local diff files as links.
+
+    The ComparisonResults entry for |image_name| should have its *_image fields
+    filled after this unless corresponding images were not found on disk.
+
+    Args:
+      image_name: A string containing the name of the image that was diffed.
+      output_manager: An output manager used used to surface links to users,
+          if necessary. The expected argument type depends on each subclasses'
+          implementation of this method.
+      output_dir: A string containing the path to the directory where diff
+          output image files where saved.
+    """
+    raise NotImplementedError()
+
+  @staticmethod
+  def _RunCmdForRcAndOutput(cmd):
+    """Runs |cmd| and returns its returncode and output.
+
+    Args:
+      cmd: A list containing the command line to run.
 
-def _RunCmdForRcAndOutput(cmd):
-  try:
-    output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
-    return 0, output
-  except subprocess.CalledProcessError as e:
-    return e.returncode, e.output
+    Returns:
+      A tuple (rc, output), where, |rc| is the returncode of the command and
+      |output| is the stdout + stderr of the command.
+    """
+    raise NotImplementedError()

content/test/gpu/gpu_tests/skia_gold/skia_gold_session_manager.py → testing/skia_gold_common/skia_gold_session_manager.py

 # Copyright 2020 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.
-"""Class for managing multiple SkiaGoldSessions.
-
-This is vased heavily off Android's Skia Gold implementation in
-//build/android/pylib/utils/gold_utils.py. If you need to make a change to this
-file, check to see if the same change needs to be made there.
-"""
+"""Class for managing multiple SkiaGoldSessions."""
 
 import json
 import tempfile
 
-from gpu_tests.skia_gold import skia_gold_session
-
-DEFAULT_INSTANCE = 'chrome-gpu'
-
 
 class SkiaGoldSessionManager(object):
   def __init__(self, working_dir, gold_properties):
-    """Class to manage one or more skia_gold_session.SkiaGoldSessions.
+    """Abstract class to manage one or more skia_gold_session.SkiaGoldSessions.
 
     A separate session is required for each instance/corpus/keys_file
     combination, so this class will lazily create them as necessary.
@@ -33,22 +24,23 @@ class SkiaGoldSessionManager(object):
     self._gold_properties = gold_properties
     self._sessions = {}
 
-  def GetSkiaGoldSession(self,
-                         keys_dict,
-                         corpus=None,
-                         instance=DEFAULT_INSTANCE):
+  def GetSkiaGoldSession(self, keys_input, corpus=None, instance=None):
     """Gets a SkiaGoldSession for the given arguments.
 
     Lazily creates one if necessary.
 
     Args:
-      keys_dict: A dictionary containing various comparison config data such as
+      keys_input: A way of retrieving various comparison config data such as
           corpus and debug information like the hardware/software configuration
-          the image was produced on.
-      corpus: The corpus the session is for. If None, the corpus will be
-          determined using available information.
-      instance: The name of the Skia Gold instance to interact with.
+          the image was produced on. Can be either a dict or a filepath to a
+          file containing JSON to read.
+      corpus: A string containing the corpus the session is for. If None, the
+          corpus will be determined using available information.
+      instance: The name of the Skia Gold instance to interact with. It None,
+          will use whatever default the subclass sets.
     """
+    instance = instance or self._GetDefaultInstance()
+    keys_dict = _GetKeysAsDict(keys_input)
     keys_string = json.dumps(keys_dict, sort_keys=True)
     if corpus is None:
       corpus = keys_dict.get('source_type', instance)
@@ -59,13 +51,65 @@ class SkiaGoldSessionManager(object):
                                             keys_string, None)
     if not session:
       working_dir = tempfile.mkdtemp(dir=self._working_dir)
-      keys_file = tempfile.NamedTemporaryFile(suffix='.json',
-                                              dir=working_dir,
-                                              delete=False).name
-      with open(keys_file, 'w') as f:
-        json.dump(keys_dict, f)
-      session = skia_gold_session.SkiaGoldSession(working_dir,
-                                                  self._gold_properties,
-                                                  keys_file, corpus, instance)
+      keys_file = _GetKeysAsJson(keys_input, working_dir)
+      session = self._GetSessionClass()(working_dir, self._gold_properties,
+                                        keys_file, corpus, instance)
       self._sessions[instance][corpus][keys_string] = session
     return session
+
+  @staticmethod
+  def _GetDefaultInstance():
+    """Gets the default Skia Gold instance.
+
+    Returns:
+      A string containing the default instance.
+    """
+    raise NotImplementedError
+
+  @staticmethod
+  def _GetSessionClass():
+    """Gets the SkiaGoldSession class to use for session creation.
+
+    Returns:
+      A reference to a SkiaGoldSession class.
+    """
+    raise NotImplementedError
+
+
+def _GetKeysAsDict(keys_input):
+  """Converts |keys_input| into a dictionary.
+
+  Args:
+    keys_input: A dictionary or a string pointing to a JSON file. The contents
+        of either should be Skia Gold config data.
+
+  Returns:
+    A dictionary containing the Skia Gold config data.
+  """
+  if isinstance(keys_input, dict):
+    return keys_input
+  assert isinstance(keys_input, str)
+  with open(keys_input) as f:
+    return json.load(f)
+
+
+def _GetKeysAsJson(keys_input, session_work_dir):
+  """Converts |keys_input| into a JSON file on disk.
+
+  Args:
+    keys_input: A dictionary or a string pointing to a JSON file. The contents
+        of either should be Skia Gold config data.
+
+  Returns:
+    A string containing a filepath to a JSON file with containing |keys_input|'s
+    data.
+  """
+  if isinstance(keys_input, str):
+    return keys_input
+  assert isinstance(keys_input, dict)
+  keys_file = tempfile.NamedTemporaryFile(suffix='.json',
+                                          dir=session_work_dir,
+                                          delete=False).name
+  with open(keys_file, 'w') as f:
+    json.dump(keys_input, f)
+  return keys_file

content/test/gpu/gpu_tests/skia_gold/skia_gold_session_manager_unittest.py → testing/skia_gold_common/skia_gold_session_manager_unittest.py

@@ -5,19 +5,20 @@
 
 #pylint: disable=protected-access
 
+import json
 import os
 import tempfile
 import unittest
 
 import mock
 
-from gpu_tests.skia_gold import skia_gold_properties
-from gpu_tests.skia_gold import skia_gold_session
-from gpu_tests.skia_gold import skia_gold_session_manager
-from gpu_tests.skia_gold import unittest_utils
-
 from pyfakefs import fake_filesystem_unittest
 
+from skia_gold_common import skia_gold_properties
+from skia_gold_common import skia_gold_session
+from skia_gold_common import skia_gold_session_manager
+from skia_gold_common import unittest_utils
+
 createSkiaGoldArgs = unittest_utils.createSkiaGoldArgs
 
 
@@ -27,6 +28,11 @@ class SkiaGoldSessionManagerGetSessionTest(fake_filesystem_unittest.TestCase):
   def setUp(self):
     self.setUpPyfakefs()
     self._working_dir = tempfile.mkdtemp()
+    self._patcher = mock.patch.object(
+        skia_gold_session_manager.SkiaGoldSessionManager, '_GetSessionClass')
+    self._session_class_mock = self._patcher.start()
+    self._session_class_mock.return_value = skia_gold_session.SkiaGoldSession
+    self.addCleanup(self._patcher.stop)
 
   def test_ArgsForwardedToSession(self):
     args = createSkiaGoldArgs()
@@ -62,6 +68,19 @@ class SkiaGoldSessionManagerGetSessionTest(fake_filesystem_unittest.TestCase):
     self.assertEqual(session._corpus, 'instance')
     self.assertEqual(session._instance, 'instance')
 
+  @mock.patch.object(skia_gold_session_manager.SkiaGoldSessionManager,
+                     '_GetDefaultInstance')
+  def test_getDefaultInstance(self, default_instance_mock):
+    default_instance_mock.return_value = 'default'
+    args = createSkiaGoldArgs()
+    sgp = skia_gold_properties.SkiaGoldProperties(args)
+    sgsm = skia_gold_session_manager.SkiaGoldSessionManager(
+        self._working_dir, sgp)
+    session = sgsm.GetSkiaGoldSession({}, None, None)
+    self.assertTrue(session._keys_file.startswith(self._working_dir))
+    self.assertEqual(session._corpus, 'default')
+    self.assertEqual(session._instance, 'default')
+
   @mock.patch.object(skia_gold_session.SkiaGoldSession, '__init__')
   def test_matchingSessionReused(self, session_mock):
     session_mock.return_value = None
@@ -115,5 +134,43 @@ class SkiaGoldSessionManagerGetSessionTest(fake_filesystem_unittest.TestCase):
     self.assertEqual(session_mock.call_count, 2)
 
 
+class SkiaGoldSessionManagerKeyConversionTest(fake_filesystem_unittest.TestCase
+                                              ):
+  def setUp(self):
+    self.setUpPyfakefs()
+    self._working_dir = tempfile.mkdtemp()
+
+  def test_getKeysAsDict(self):
+    keys_dict = {'foo': 'bar'}
+    keys_file_contents = {'bar': 'baz'}
+    keys_file = tempfile.NamedTemporaryFile(delete=False).name
+    with open(keys_file, 'w') as f:
+      json.dump(keys_file_contents, f)
+
+    self.assertEqual(skia_gold_session_manager._GetKeysAsDict(keys_dict),
+                     keys_dict)
+    self.assertEqual(skia_gold_session_manager._GetKeysAsDict(keys_file),
+                     keys_file_contents)
+    with self.assertRaises(AssertionError):
+      skia_gold_session_manager._GetKeysAsDict(1)
+
+  def test_getKeysAsJson(self):
+    keys_dict = {'foo': 'bar'}
+    keys_file_contents = {'bar': 'baz'}
+    keys_file = tempfile.NamedTemporaryFile(delete=False).name
+    with open(keys_file, 'w') as f:
+      json.dump(keys_file_contents, f)
+
+    self.assertEqual(skia_gold_session_manager._GetKeysAsJson(keys_file, None),
+                     keys_file)
+    keys_dict_as_json = skia_gold_session_manager._GetKeysAsJson(
+        keys_dict, self._working_dir)
+    self.assertTrue(keys_dict_as_json.startswith(self._working_dir))
+    with open(keys_dict_as_json) as f:
+      self.assertEqual(json.load(f), keys_dict)
+    with self.assertRaises(AssertionError):
+      skia_gold_session_manager._GetKeysAsJson(1, None)
+
+
 if __name__ == '__main__':
   unittest.main(verbosity=2)