Report public and internal Gold links

Makes the Android instrumentation tests and GPU pixel tests report both
a public and internal Gold triage link. These links should be equivalent
for most use cases, but in cases where there are internal results (such
as when testing on an internal repo), such results will only be
available on the internal instance if logged in with an @google.com
account.

Bug: 1077274
Change-Id: I7d1964b9fdbce27860f6111a35184c82da16d308
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2285323Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Auto-Submit: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786259}

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

@@ -1077,8 +1077,9 @@ class LocalDeviceInstrumentationTestRun(
           _AppendToLog(results,
                        'Gold initialization failed with output %s' % error)
         elif status == status_codes.COMPARISON_FAILURE_REMOTE:
-          triage_link = gold_session.GetTriageLink(render_name)
-          if not triage_link:
+          public_triage_link, internal_triage_link =\
+              gold_session.GetTriageLinks(render_name)
+          if not public_triage_link:
             _AppendToLog(
                 results, 'Failed to get triage link for %s, raw output: %s' %
                 (render_name, error))
@@ -1087,12 +1088,19 @@ class LocalDeviceInstrumentationTestRun(
                 gold_session.GetTriageLinkOmissionReason(render_name))
             continue
           if gold_properties.IsTryjobRun():
-            _SetLinkOnResults(results, 'Skia Gold triage link for entire CL',
-                              triage_link)
-          else:
             _SetLinkOnResults(results,
-                              'Skia Gold triage link for %s' % render_name,
-                              triage_link)
+                              'Public Skia Gold triage link for entire CL',
+                              public_triage_link)
+            _SetLinkOnResults(results,
+                              'Internal Skia Gold triage link for entire CL',
+                              internal_triage_link)
+          else:
+            _SetLinkOnResults(
+                results, 'Public Skia Gold triage link for %s' % render_name,
+                public_triage_link)
+            _SetLinkOnResults(
+                results, 'Internal Skia Gold triage link for %s' % render_name,
+                internal_triage_link)
           _AppendToLog(results, failure_log)
 
         elif status == status_codes.COMPARISON_FAILURE_LOCAL:

build/skia_gold_common/skia_gold_session.py

@@ -36,7 +36,8 @@ class SkiaGoldSession(object):
     """Struct-like object for storing results of an image comparison."""
 
     def __init__(self):
-      self.triage_link = None
+      self.public_triage_link = None
+      self.internal_triage_link = None
       self.triage_link_omission_reason = None
       self.local_diff_given_image = None
       self.local_diff_closest_image = None
@@ -224,7 +225,7 @@ class SkiaGoldSession(object):
   def Compare(self, name, png_file, inexact_matching_args=None):
     """Compares the given image to images known to Gold.
 
-    Triage links can later be retrieved using GetTriageLink().
+    Triage links can later be retrieved using GetTriageLinks().
 
     Args:
       name: The name of the image being compared.
@@ -274,12 +275,16 @@ class SkiaGoldSession(object):
           instance=self._instance,
           crs=self._gold_properties.code_review_system,
           issue=self._gold_properties.issue)
-      self._comparison_results[name].triage_link = cl_triage_link
+      self._comparison_results[name].internal_triage_link = cl_triage_link
+      self._comparison_results[name].public_triage_link =\
+          self._GeneratePublicTriageLink(cl_triage_link)
     else:
       try:
         with open(self._triage_link_file) as tlf:
           triage_link = tlf.read().strip()
-        self._comparison_results[name].triage_link = triage_link
+        self._comparison_results[name].internal_triage_link = triage_link
+        self._comparison_results[name].public_triage_link =\
+            self._GeneratePublicTriageLink(triage_link)
       except IOError:
         self._comparison_results[name].triage_link_omission_reason = (
             'Failed to read triage link from file')
@@ -333,19 +338,23 @@ class SkiaGoldSession(object):
     self._StoreDiffLinks(name, output_manager, output_dir)
     return rc, stdout
 
-  def GetTriageLink(self, name):
-    """Gets the triage link for the given image.
+  def GetTriageLinks(self, name):
+    """Gets the triage links for the given image.
 
     Args:
       name: The name of the image to retrieve the triage link for.
 
     Returns:
-      A string containing the triage link if it is available, or None if it is
-      not available for some reason. The reason can be retrieved using
-      GetTriageLinkOmissionReason.
+      A tuple (public, internal). |public| is a string containing the triage
+      link for the public Gold instance if it is available, or None if it is not
+      available for some reason. |internal| is the same as |public|, but
+      containing a link to the internal Gold instance. The reason for links not
+      being available can be retrieved using GetTriageLinkOmissionReason.
     """
-    return self._comparison_results.get(name,
-                                        self.ComparisonResults()).triage_link
+    comparison_results = self._comparison_results.get(name,
+                                                      self.ComparisonResults())
+    return (comparison_results.public_triage_link,
+            comparison_results.internal_triage_link)
 
   def GetTriageLinkOmissionReason(self, name):
     """Gets the reason why a triage link is not available for an image.
@@ -360,7 +369,8 @@ class SkiaGoldSession(object):
       return 'No image comparison performed for %s' % name
     results = self._comparison_results[name]
     # This method should not be called if there is a valid triage link.
-    assert results.triage_link is None
+    assert results.public_triage_link is None
+    assert results.internal_triage_link is None
     if results.triage_link_omission_reason:
       return results.triage_link_omission_reason
     if results.local_diff_given_image:
@@ -408,6 +418,20 @@ class SkiaGoldSession(object):
     assert name in self._comparison_results
     return self._comparison_results[name].local_diff_diff_image
 
+  def _GeneratePublicTriageLink(self, internal_link):
+    """Generates a public triage link given an internal one.
+
+    Args:
+      internal_link: A string containing a triage link pointing to an internal
+          Gold instance.
+
+    Returns:
+      A string containing a triage link pointing to the public mirror of the
+      link pointed to by |internal_link|.
+    """
+    return internal_link.replace('%s-gold' % self._instance,
+                                 '%s-public-gold' % self._instance)
+
   def _ClearTriageLinkFile(self):
     """Clears the contents of the triage link file.
 

build/skia_gold_common/skia_gold_session_unittest.py

@@ -542,9 +542,10 @@ class SkiaGoldSessionCompareTest(fake_filesystem_unittest.TestCase):
                                                 'keys_file', None, None)
     rc, _ = session.Compare('name', 'png_file')
     self.assertEqual(rc, 0)
-    self.assertEqual(session._comparison_results['name'].triage_link, None)
-    self.assertNotEqual(
-        session._comparison_results['name'].triage_link_omission_reason, None)
+    comparison_result = session._comparison_results['name']
+    self.assertEqual(comparison_result.public_triage_link, None)
+    self.assertEqual(comparison_result.internal_triage_link, None)
+    self.assertNotEqual(comparison_result.triage_link_omission_reason, None)
 
   @mock.patch.object(skia_gold_session.SkiaGoldSession, '_RunCmdForRcAndOutput')
   def test_clLinkOnTrybot(self, cmd_mock):
@@ -561,31 +562,46 @@ class SkiaGoldSessionCompareTest(fake_filesystem_unittest.TestCase):
                                                 instance='instance')
     rc, _ = session.Compare('name', 'png_file')
     self.assertEqual(rc, 1)
-    self.assertNotEqual(session._comparison_results['name'].triage_link, None)
-    self.assertEqual(session._comparison_results['name'].triage_link,
-                     'https://instance-gold.skia.org/cl/gerrit/1')
-    self.assertEqual(
-        session._comparison_results['name'].triage_link_omission_reason, None)
+    comparison_result = session._comparison_results['name']
+    self.assertNotEqual(comparison_result.public_triage_link, None)
+    self.assertNotEqual(comparison_result.internal_triage_link, None)
+    internal_link = 'https://instance-gold.skia.org/cl/gerrit/1'
+    public_link = 'https://instance-public-gold.skia.org/cl/gerrit/1'
+    self.assertEqual(comparison_result.internal_triage_link, internal_link)
+    self.assertEqual(comparison_result.public_triage_link, public_link)
+    self.assertEqual(comparison_result.triage_link_omission_reason, None)
+    self.assertEqual(session.GetTriageLinks('name'),
+                     (public_link, internal_link))
 
   @mock.patch.object(skia_gold_session.SkiaGoldSession, '_RunCmdForRcAndOutput')
   def test_individualLinkOnCi(self, cmd_mock):
     args = createSkiaGoldArgs(git_revision='a')
     sgp = skia_gold_properties.SkiaGoldProperties(args)
-    session = skia_gold_session.SkiaGoldSession(self._working_dir, sgp,
-                                                'keys_file', None, None)
+    session = skia_gold_session.SkiaGoldSession(self._working_dir,
+                                                sgp,
+                                                'keys_file',
+                                                None,
+                                                instance='foobar')
+
+    internal_link = 'foobar-gold.skia.org'
+    public_link = 'foobar-public-gold.skia.org'
 
     def WriteTriageLinkFile(_):
       with open(session._triage_link_file, 'w') as f:
-        f.write('foobar')
+        f.write(internal_link)
       return (1, None)
 
     cmd_mock.side_effect = WriteTriageLinkFile
     rc, _ = session.Compare('name', 'png_file')
     self.assertEqual(rc, 1)
-    self.assertNotEqual(session._comparison_results['name'].triage_link, None)
-    self.assertEqual(session._comparison_results['name'].triage_link, 'foobar')
-    self.assertEqual(
-        session._comparison_results['name'].triage_link_omission_reason, None)
+    comparison_result = session._comparison_results['name']
+    self.assertNotEqual(comparison_result.public_triage_link, None)
+    self.assertNotEqual(comparison_result.internal_triage_link, None)
+    self.assertEqual(comparison_result.internal_triage_link, internal_link)
+    self.assertEqual(comparison_result.public_triage_link, public_link)
+    self.assertEqual(comparison_result.triage_link_omission_reason, None)
+    self.assertEqual(session.GetTriageLinks('name'),
+                     (public_link, internal_link))
 
   @mock.patch.object(skia_gold_session.SkiaGoldSession, '_RunCmdForRcAndOutput')
   def test_validOmissionOnIoError(self, cmd_mock):
@@ -602,12 +618,12 @@ class SkiaGoldSessionCompareTest(fake_filesystem_unittest.TestCase):
     cmd_mock.side_effect = DeleteTriageLinkFile
     rc, _ = session.Compare('name', 'png_file')
     self.assertEqual(rc, 1)
-    self.assertEqual(session._comparison_results['name'].triage_link, None)
-    self.assertNotEqual(
-        session._comparison_results['name'].triage_link_omission_reason, None)
-    self.assertIn(
-        'Failed to read',
-        session._comparison_results['name'].triage_link_omission_reason)
+    comparison_result = session._comparison_results['name']
+    self.assertEqual(comparison_result.public_triage_link, None)
+    self.assertEqual(comparison_result.internal_triage_link, None)
+    self.assertNotEqual(comparison_result.triage_link_omission_reason, None)
+    self.assertIn('Failed to read',
+                  comparison_result.triage_link_omission_reason)
 
 
 class SkiaGoldSessionDiffTest(fake_filesystem_unittest.TestCase):
@@ -677,7 +693,12 @@ class SkiaGoldSessionTriageLinkOmissionTest(fake_filesystem_unittest.TestCase):
 
   def test_onlyWithoutTriageLink(self):
     session = self._CreateSession()
-    session._comparison_results['foo'].triage_link = 'bar'
+    comparison_result = session._comparison_results['foo']
+    comparison_result.public_triage_link = 'bar'
+    with self.assertRaises(AssertionError):
+      session.GetTriageLinkOmissionReason('foo')
+    comparison_result.public_triage_link = None
+    comparison_result.internal_triage_link = 'bar'
     with self.assertRaises(AssertionError):
       session.GetTriageLinkOmissionReason('foo')
 

content/test/gpu/gpu_tests/skia_gold_integration_test_base.py

@@ -430,7 +430,10 @@ class SkiaGoldIntegrationTestBase(gpu_integration_test.GpuIntegrationTest):
     elif status == status_codes.INIT_FAILURE:
       logging.error('Gold initialization failed with output %s', error)
     elif status == status_codes.COMPARISON_FAILURE_REMOTE:
-      triage_link = gold_session.GetTriageLink(image_name)
+      # We currently don't have an internal instance + public mirror like the
+      # general Chrome Gold instance, so just report the "internal" link, which
+      # points to the correct instance.
+      _, triage_link = gold_session.GetTriageLinks(image_name)
       if not triage_link:
         logging.error('Failed to get triage link for %s, raw output: %s',
                       image_name, error)

ui/android/javatests/src/org/chromium/ui/test/util/RENDER_TESTS.md

@@ -23,7 +23,16 @@ the `chrome_public_test_apk` step to go to the **Suites Summary** page.
 failing.
 3. On the **Test Results of Suite** page, follow the links in the **log** column
 corresponding to the renders mentioned in the failure stack trace. The links
-will be named "Skia Gold triage link for entire CL".
+will be named "[Public|Internal] Skia Gold triage link for entire CL".
+
+In most cases, the public and internal links are equivalent. The difference is:
+1. The internal link will not show any results unless logged in with an
+@google.com account.
+2. The internal link will show internal-only results, e.g. from an internal
+repo.
+
+So, unless you're working on an internal repo or otherwise expect results to
+be marked as internal-only, the public link should be fine.
 
 Once on the triage page, make sure you are logged in at the top-right.
 Currently, only @google.com and @chromium.org accounts work, but other domains
@@ -59,7 +68,8 @@ differences:
 
 1. You must manually find the triage links, as Gold has nowhere to post a
 comment to. Alternatively, you can check for untriaged images directly in the
-[gold instance](https://chrome-gold.skia.org).
+[public gold instance](https://chrome-public-gold.skia.org) or
+[internal gold instance](https://chrome-gold.skia.org).
 2. Triage links are for specific images instead of for an entire CL, and are
 thus named after the render name.