Hide flaky RenderTests on trybots

Adds an ignore:1 to the JSON uploaded to Gold from the Android
RenderTests if we are running on a trybot and it is not the final
retry attempt. This is to counter flaky tests causing unrelated CLs to
receive comments from Gold when the test causes their CL to produce a
new image. With this change, CLs will only receive a comment if a
test fails consistently.

Bug: skia:10787
Change-Id: I827de961e49a357f1755d6931b89468e27bebac6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446717Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#814807}

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

@@ -100,6 +100,7 @@ class LocalDeviceEnvironment(environment.Environment):
 
   def __init__(self, args, output_manager, _error_func):
     super(LocalDeviceEnvironment, self).__init__(output_manager)
+    self._current_try = 0
     self._denylist = (device_denylist.Denylist(args.denylist_file)
                       if args.denylist_file else None)
     self._device_serials = args.test_devices
@@ -188,6 +189,13 @@ class LocalDeviceEnvironment(environment.Environment):
 
     self.parallel_devices.pMap(prepare_device)
 
+  @property
+  def current_try(self):
+    return self._current_try
+
+  def IncrementCurrentTry(self):
+    self._current_try += 1
+
   @property
   def denylist(self):
     return self._denylist

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

@@ -1037,6 +1037,10 @@ class LocalDeviceInstrumentationTestRun(
         # that implies that we aren't actively maintaining baselines for the
         # test. This helps prevent unrelated CLs from getting comments posted to
         # them.
+        # Additionally, add the ignore if we're running on a trybot and this is
+        # not our final retry attempt in order to prevent unrelated CLs from
+        # getting spammed if a test is flaky.
+        optional_keys = {}
         with open(json_path) as infile:
           # All the key/value pairs in the JSON file are strings, so convert
           # to a bool.
@@ -1044,13 +1048,20 @@ class LocalDeviceInstrumentationTestRun(
           fail_on_unsupported = json_dict.get('fail_on_unsupported_configs',
                                               'false')
           fail_on_unsupported = fail_on_unsupported.lower() == 'true'
-        should_hide_failure = (
+        running_on_unsupported = (
             device.build_version_sdk not in RENDER_TEST_MODEL_SDK_CONFIGS.get(
                 device.product_model, []) and not fail_on_unsupported)
+        # TODO(skbug.com/10787): Remove the ignore on non-final retry once we
+        # fully switch over to using the Gerrit plugin for surfacing Gold
+        # information since it does not spam people with emails due to automated
+        # comments.
+        not_final_retry = self._env.current_try + 1 != self._env.max_tries
+        tryjob_but_not_final_retry =\
+            not_final_retry and gold_properties.IsTryjobRun()
+        should_hide_failure =\
+            running_on_unsupported or tryjob_but_not_final_retry
         if should_hide_failure:
-          json_dict['ignore'] = '1'
-          with open(json_path, 'w') as outfile:
-            json.dump(json_dict, outfile)
+          optional_keys['ignore'] = '1'
 
         gold_session = self._skia_gold_session_manager.GetSkiaGoldSession(
             keys_input=json_path)
@@ -1060,7 +1071,8 @@ class LocalDeviceInstrumentationTestRun(
               name=render_name,
               png_file=image_path,
               output_manager=self._env.output_manager,
-              use_luci=use_luci)
+              use_luci=use_luci,
+              optional_keys=optional_keys)
         except Exception as e:  # pylint: disable=broad-except
           _FailTestIfNecessary(results)
           _AppendToLog(results, 'Skia Gold comparison raised exception: %s' % e)

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

@@ -135,8 +135,8 @@ class LocalDeviceTestRun(test_run.TestRun):
 
     try:
       with signal_handler.AddSignalHandler(signal.SIGTERM, stop_tests):
-        tries = 0
-        while tries < self._env.max_tries and tests:
+        while self._env.current_try < self._env.max_tries and tests:
+          tries = self._env.current_try
           grouped_tests = self._GroupTests(tests)
           logging.info('STARTING TRY #%d/%d', tries + 1, self._env.max_tries)
           if tries > 0 and self._env.recover_devices:
@@ -189,7 +189,7 @@ class LocalDeviceTestRun(test_run.TestRun):
                       log=_SIGTERM_TEST_LOG))
             raise
 
-          tries += 1
+          self._env.IncrementCurrentTry()
           tests = self._GetTestsToRetry(tests, try_results)
 
           logging.info('FINISHED TRY #%d/%d', tries, self._env.max_tries)

build/skia_gold_common/output_managerless_skia_gold_session.py

@@ -20,7 +20,8 @@ class OutputManagerlessSkiaGoldSession(skia_gold_session.SkiaGoldSession):
       png_file,
       output_manager=True,
       inexact_matching_args=None,
-      use_luci=True):
+      use_luci=True,
+      optional_keys=None):
     # 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.
@@ -29,7 +30,8 @@ class OutputManagerlessSkiaGoldSession(skia_gold_session.SkiaGoldSession):
         png_file=png_file,
         output_manager=output_manager,
         inexact_matching_args=inexact_matching_args,
-        use_luci=use_luci)
+        use_luci=use_luci,
+        optional_keys=optional_keys)
 
   def _CreateDiffOutputDir(self):
     # We intentionally don't clean this up and don't put it in self._working_dir

build/skia_gold_common/skia_gold_session.py

@@ -82,7 +82,8 @@ class SkiaGoldSession(object):
                     png_file,
                     output_manager,
                     inexact_matching_args=None,
-                    use_luci=True):
+                    use_luci=True,
+                    optional_keys=None):
     """Helper method to run all steps to compare a produced image.
 
     Handles authentication, itnitialization, comparison, and, if necessary,
@@ -101,6 +102,10 @@ class SkiaGoldSession(object):
       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.
+      optional_keys: A dict containing optional key/value pairs to pass to Gold
+          for this comparison. Optional keys are keys unrelated to the
+          configuration the image was produced on, e.g. a comment or whether
+          Gold should treat the image as ignored.
 
     Returns:
       A tuple (status, error). |status| is a value from
@@ -118,7 +123,8 @@ class SkiaGoldSession(object):
     compare_rc, compare_stdout = self.Compare(
         name=name,
         png_file=png_file,
-        inexact_matching_args=inexact_matching_args)
+        inexact_matching_args=inexact_matching_args,
+        optional_keys=optional_keys)
     if not compare_rc:
       return self.StatusCodes.SUCCESS, None
 
@@ -227,7 +233,11 @@ class SkiaGoldSession(object):
       self._initialized = True
     return rc, stdout
 
-  def Compare(self, name, png_file, inexact_matching_args=None):
+  def Compare(self,
+              name,
+              png_file,
+              inexact_matching_args=None,
+              optional_keys=None):
     """Compares the given image to images known to Gold.
 
     Triage links can later be retrieved using GetTriageLinks().
@@ -238,6 +248,10 @@ class SkiaGoldSession(object):
       inexact_matching_args: A list of strings containing extra command line
           arguments to pass to Gold for inexact matching. Can be omitted to use
           exact matching.
+      optional_keys: A dict containing optional key/value pairs to pass to Gold
+          for this comparison. Optional keys are keys unrelated to the
+          configuration the image was produced on, e.g. a comment or whether
+          Gold should treat the image as ignored.
 
     Returns:
       A tuple (return_code, output). |return_code| is the return code of the
@@ -267,6 +281,13 @@ class SkiaGoldSession(object):
                    inexact_matching_args)
       compare_cmd.extend(inexact_matching_args)
 
+    optional_keys = optional_keys or {}
+    for k, v in optional_keys.iteritems():
+      compare_cmd.extend([
+          '--add-test-optional-key',
+          '%s:%s' % (k, v),
+      ])
+
     self._ClearTriageLinkFile()
     rc, stdout = self._RunCmdForRcAndOutput(compare_cmd)
 

build/skia_gold_common/skia_gold_session_unittest.py

@@ -164,7 +164,37 @@ class SkiaGoldSessionRunComparisonTest(fake_filesystem_unittest.TestCase):
     self.assertEqual(diff_mock.call_count, 0)
     compare_mock.assert_called_with(name=None,
                                     png_file=mock.ANY,
-                                    inexact_matching_args=['--inexact'])
+                                    inexact_matching_args=['--inexact'],
+                                    optional_keys=None)
+
+  @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Diff')
+  @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Compare')
+  @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Initialize')
+  @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Authenticate')
+  def test_compareOptionalKeys(self, auth_mock, init_mock, compare_mock,
+                               diff_mock):
+    auth_mock.return_value = (0, None)
+    init_mock.return_value = (0, None)
+    compare_mock.return_value = (0, None)
+    diff_mock.return_value = (0, None)
+    args = createSkiaGoldArgs(local_pixel_tests=False)
+    sgp = skia_gold_properties.SkiaGoldProperties(args)
+    session = skia_gold_session.SkiaGoldSession(self._working_dir, sgp,
+                                                self._json_keys, None, None)
+    status, _ = session.RunComparison(None,
+                                      None,
+                                      None,
+                                      optional_keys={'foo': 'bar'})
+    self.assertEqual(status,
+                     skia_gold_session.SkiaGoldSession.StatusCodes.SUCCESS)
+    self.assertEqual(auth_mock.call_count, 1)
+    self.assertEqual(init_mock.call_count, 1)
+    self.assertEqual(compare_mock.call_count, 1)
+    self.assertEqual(diff_mock.call_count, 0)
+    compare_mock.assert_called_with(name=None,
+                                    png_file=mock.ANY,
+                                    inexact_matching_args=None,
+                                    optional_keys={'foo': 'bar'})
 
   @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Diff')
   @mock.patch.object(skia_gold_session.SkiaGoldSession, 'Compare')
@@ -655,6 +685,17 @@ class SkiaGoldSessionCompareTest(fake_filesystem_unittest.TestCase):
     self.assertIn('Failed to read',
                   comparison_result.triage_link_omission_reason)
 
+  @mock.patch.object(skia_gold_session.SkiaGoldSession, '_RunCmdForRcAndOutput')
+  def test_optionalKeysPassedToGoldctl(self, cmd_mock):
+    cmd_mock.return_value = (None, None)
+    args = createSkiaGoldArgs(git_revision='a', local_pixel_tests=True)
+    sgp = skia_gold_properties.SkiaGoldProperties(args)
+    session = skia_gold_session.SkiaGoldSession(self._working_dir, sgp,
+                                                self._json_keys, None, None)
+    session.Compare(None, None, optional_keys={'foo': 'bar'})
+    assertArgWith(self, cmd_mock.call_args[0][0], '--add-test-optional-key',
+                  'foo:bar')
+
 
 class SkiaGoldSessionDiffTest(fake_filesystem_unittest.TestCase):
   """Tests the functionality of SkiaGoldSession.Diff."""