Only report Patch failure on infra error and refactor code

appengine/chromium_try_flakes/handlers/flake_issues.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.
 
 """Task queue endpoints for creating and updating issues on issue tracker."""
 
 import datetime
 import json
 import logging
 import urllib2
@@ skipping 51 matching lines @@
     'https://chromium-try-flakes.appspot.com/all_flake_occurrences?key=%s')
 TEST_RESULTS_URL_TEMPLATE = (
     'http://test-results.appspot.com/testfile?builder=%(buildername)s&name='
     'full_results.json&master=%(mastername)s&testtype=%(stepname)s&buildnumber='
     '%(buildnumber)s')
 VERY_STALE_FLAKES_MESSAGE = (
     'Reporting to stale-flakes-reports@google.com to investigate why this '
     'issue is not being processed despite being in an appropriate queue.')
 STALE_FLAKES_ML = 'stale-flakes-reports@google.com'
 MAX_GAP_FOR_FLAKINESS_PERIOD = datetime.timedelta(days=3)
-KNOWN_TROOPER_FAILURES = [
+KNOWN_TROOPER_FLAKE_NAMES = [
     'analyze', 'bot_update', 'compile (with patch)', 'compile',
     'device_status_check', 'gclient runhooks (with patch)', 'Patch failure',
     'process_dumps', 'provision_devices', 'update_scripts']
 
+# Flakes in these steps are always ignored:
+#  - steps: always red when any other step is red (duplicates failure)
+#  - presubmit: typically red due to missing OWNERs LGTM, not a flake
+#  - recipe failure reason: always red when build fails (not a failure)
+#  - test results: always red when another step is red (not a failure)
+#  - Uncaught Exception: summary step referring to an exception in another
+#    step (duplicates failure)
+# There are additional rules for non-trivial cases in the FlakyRun.post method.
+IGNORED_STEPS = ['steps', 'presubmit', 'recipe failure reason', 'test results',
+                 'Uncaught Exception']
+
 
 def is_trooper_flake(flake_name):
-  return flake_name in KNOWN_TROOPER_FAILURES
+  return flake_name in KNOWN_TROOPER_FLAKE_NAMES
 
 
 def get_queue_details(flake_name):
   if is_trooper_flake(flake_name):
     return 'Trooper Bug Queue', 'Infra-Troopers'
   else:
     return 'Sheriff Bug Queue', 'Sheriff-Chromium'
 
 
 class ProcessIssue(webapp2.RequestHandler):
@@ skipping 374 matching lines @@
 
     failure_run = ndb.Key(urlsafe=self.request.get('failure_run_key')).get()
     success_run = ndb.Key(urlsafe=self.request.get('success_run_key')).get()
 
     flaky_run = FlakyRun(
         failure_run=failure_run.key,
         failure_run_time_started=failure_run.time_started,
         failure_run_time_finished=failure_run.time_finished,
         success_run=success_run.key)
 
-    success_time = success_run.time_finished
     failure_time = failure_run.time_finished
     patchset_builder_runs = failure_run.key.parent().get()
 
     # TODO(sergiyb): The parsing logic below is very fragile and will break with
     # any changes to step names and step text. We should move away from parsing
     # buildbot to tools like flakiness dashboard (test-results.appspot.com),
     # which uses a standartized JSON format.
     url = ('http://build.chromium.org/p/' + patchset_builder_runs.master +
            '/json/builders/' + patchset_builder_runs.builder +'/builds/' +
            str(failure_run.buildnumber))
@@ skipping 11 matching lines @@
     passed_steps = []
     for step in steps:
       result = step['results'][0]
       if build_result.isResultSuccess(result):
         passed_steps.append(step)
         continue
       if not build_result.isResultFailure(result):
         continue
       step_name = step['name']
       step_text = ' '.join(step['text'])
-      # The following step failures are ignored:
-      #  - steps: always red when any other step is red (not a failure)
+      if step_name in IGNORED_STEPS:
+        continue
+
+      # Custom (non-trivial) rules for ignoring flakes in certain steps:
       #  - [swarming] ...: summary step would also be red (do not double count)
-      #  - presubmit: typically red due to missing OWNERs LGTM, not a flake
-      #  - recipe failure reason: always red when build fails (not a failure)
-      #  - Patch failure: if success run was before failure run, it is
-      #    likely a legitimate failure. For example it often happens that
-      #    developers use CQ dry run and then wait for a review. Once getting
-      #    LGTM they check CQ checkbox, but the patch does not cleanly apply
-      #    anymore.
-      #  - bot_update PATCH FAILED: Corresponds to 'Patch failure' step.
-      #  - test results: always red when another step is red (not a failure)
-      #  - Uncaught Exception: summary step referring to an exception in another
-      #    step (e.g. bot_update)
+      #  - Patch failure: ingore non-infra failures as they are typically due to
+      #    changes in the code on HEAD
+      #  - bot_update PATCH FAILED: Duplicates failure in 'Patch failure' step.
       #  - ... (retry summary): this is an artificial step to fail the build due
       #    to another step that has failed earlier (do not double count).
-      if (step_name == 'steps' or step_name.startswith('[swarming]') or
-          step_name == 'presubmit' or step_name == 'recipe failure reason' or
-          (step_name == 'Patch failure' and success_time < failure_time) or
-          (step_name == 'bot_update' and 'PATCH FAILED' in step_text) or
-          step_name == 'test results' or step_name == 'Uncaught Exception' or
-          step_name.endswith(' (retry summary)')):
+      print step_name, '1', build_result
+      if (step_name.startswith('[swarming]') or
+          step_name.endswith(' (retry summary)') or
+          (step_name == 'Patch failure' and result != build_result.EXCEPTION) or
+          (step_name == 'bot_update' and 'PATCH FAILED' in step_text)):
         continue
+      print step_name, '2'
+
       failed_steps.append(step)
 
     steps_to_ignore = []
     for step in failed_steps:
       step_name = step['name']
       if ' (with patch)' in step_name:
         # Android instrumentation tests add a prefix before the step name, which
         # doesn't appear on the summary step (without suffixes). To make sure we
         # correctly ignore duplicate failures, we remove the prefix.
         step_name = step_name.replace('Instrumentation test ', '')
@@ skipping 18 matching lines @@
           failure_run.buildnumber, step)
       for flake in flakes:
         flake_occurrence = FlakeOccurrence(name=step_name, failure=flake)
         flaky_run.flakes.append(flake_occurrence)
         flakes_to_update.append(flake)
 
     flaky_run_key = flaky_run.put()
     for flake in flakes_to_update:
       self.add_failure_to_flake(flake, flaky_run_key, failure_time)
     self.flaky_runs.increment_by(1)