[Findit] Show build analysis references in UI for Findit Cross-platform auto-triage

appengine/findit/handlers/triage_analysis.py

 # Copyright 2014 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.
 
 """This module is to handle manual triage of analysis result.
 
 This handler will flag the analysis result as correct or incorrect.
 TODO: work on an automatic or semi-automatic way to triage analysis result.
 """
 
@@ skipping 30 matching lines @@
 
     or could look like:
 
         (step_name, revision)
   """
   potential_culprit_tuple_list = []
 
   # Iterates through the failures, tests, and suspected_cls, appending potential
   # (step_name, test_name, revision) and (step_name, revision) culprit tuples to
   # the list.
+

[lijeffrey, 2016/06/27 20:38:22]

nit: delete this empty line

[josiahk, 2016/06/28 22:59:23]

Done.

   for failure in analysis.result['failures']:
     if failure.get('tests'):
       for test in failure['tests']:
         for suspected_cl in test.get('suspected_cls', []):
           potential_culprit_tuple_list.append((
               failure['step_name'],
               test['test_name'],
               suspected_cl['revision']))
     else:
       for suspected_cl in failure['suspected_cls']:
@@ skipping 22 matching lines @@
 
   # Both analyses must have non-empty potential culprit lists.
   if not potential_culprit_tuple_list_1 or not potential_culprit_tuple_list_2:
     return False
 
   # Both analyses must have matching potential culprit lists.
   return (sorted(potential_culprit_tuple_list_1) ==
           sorted(potential_culprit_tuple_list_2))
 
 
-def _AppendTriageHistoryRecord(analysis, is_correct, user_name):
+def _AppendTriageHistoryRecord(
+    analysis, is_correct, user_name, is_duplicate=False):
   """Appends a triage history record to the given analysis.
 
   Args:
     analysis: The analysis to which to append the history record.
     is_correct: True if the history record should indicate a correct judgement,
         otherwise False.
     user_name: The user_name of the person to include in the triage record.
+    is_duplicate: True if the history record should indicate that this triage is
+        a duplicate, otherwise False.
   """
   if is_correct:
     if analysis.suspected_cls:
-      analysis.result_status = result_status.FOUND_CORRECT
+      if is_duplicate:
+        analysis.result_status = result_status.FOUND_CORRECT_DUPLICATE
+      else:
+        analysis.result_status = result_status.FOUND_CORRECT
       analysis.culprit_cls = analysis.suspected_cls
     else:
       analysis.result_status = result_status.NOT_FOUND_CORRECT
       analysis.culprit_cls = None
   else:
     analysis.culprit_cls = None
     if analysis.suspected_cls:
-      analysis.result_status = result_status.FOUND_INCORRECT
+      if is_duplicate:
+        analysis.result_status = result_status.FOUND_INCORRECT_DUPLICATE
+      else:
+        analysis.result_status = result_status.FOUND_INCORRECT
     else:
       analysis.result_status = result_status.NOT_FOUND_INCORRECT
 
   triage_record = {
       'triage_timestamp': calendar.timegm(datetime.utcnow().timetuple()),
       'user_name': user_name,
       'result_status': analysis.result_status,
       'version': analysis.version,
   }
   if not analysis.triage_history:
     analysis.triage_history = []
   analysis.triage_history.append(triage_record)
 
   analysis.put()
 
 
 @ndb.transactional
 def _UpdateAnalysisResultStatus(
     master_name, builder_name, build_number, is_correct, user_name=None):
   analysis = WfAnalysis.Get(master_name, builder_name, build_number)
   if not analysis or not analysis.completed:
     return False, None
 
+  analysis.triage_reference_analysis_master_name = None

[lijeffrey, 2016/06/27 20:38:22]

so what are these for, and why are we setting them to None here?

[chanli, 2016/06/27 22:27:56]

+1

[josiahk, 2016/06/28 22:59:23]

When another "first-cause" build analysis is triaged, and this build analysis is
marked as a duplicate from that other "first-cause" build analysis, these are
the variables that hold the reference back to that "first-cause" build analysis.

It's possible that someone could then manually re-triage this build analysis, in
which case this build analysis is no longer a duplicate, and we want to erase
the reference to the no-longer-relevant "first-cause" build_analysis, which is
what these lines are doing.

Maybe it would be a good idea for me to move these lines into
_AppendTriageHistoryRecord(), because that is where other properties of the
analysis get updated.

[lijeffrey, 2016/06/29 00:48:23]

you don't need to initialize them to None, if they're not explicitly set they
should default to None so these lines are unnecessary unless you specifically
want to overwrite any previously set values

[josiahk, 2016/06/29 22:40:00]

Yes, I want to potentially overwrite previously set values. There is a
possibility they might be set to something other than None (see lines 202-207).
Thanks!

[lijeffrey, 2016/06/30 01:09:46]

It appears with what you have triage duplicate results which will do the writing
is always called after this function. If that's indeed the case then setting
these to None here would just be overwritten anyway too, making them unnecessary

[josiahk, 2016/06/30 22:34:56]

_UpdateAnalysisResultStatus() operates on a single original build analysis that
the user directly triages.
_TriageDuplicateResults() operates on potentially many build analyses that match
that first single original build analysis.

Since _TriageDuplicateResults() doesn't operate on the original build analysis,
the None's don't get overwritten right away.

I moved this code to inside _AppendTriageHistoryRecord(), and added a
conditional and a comment; hopefully the code is more self-explanatory now!

+  analysis.triage_reference_analysis_builder_name = None
+  analysis.triage_reference_analysis_build_number = None
+
   _AppendTriageHistoryRecord(analysis, is_correct, user_name)
 
   return True, analysis
 
 
 def _GetDuplicateAnalyses(original_analysis):
   start_time = (original_analysis.build_start_time -
                 timedelta(hours=MATCHING_ANALYSIS_HOURS_AGO_START))
   end_time = (original_analysis.build_start_time +
               timedelta(hours=MATCHING_ANALYSIS_HOURS_AGO_END))
@@ skipping 22 matching lines @@
 
   # Retrieve potential duplicate build analyses.
   analysis_results = WfAnalysis.query(ndb.AND(
       WfAnalysis.build_start_time >= start_time,
       WfAnalysis.build_start_time <= end_time,
       WfAnalysis.result_status == result_status.FOUND_UNTRIAGED
       )).fetch()
 
   # Further filter potential duplicates and return them.
   return [analysis for analysis in analysis_results if
+          analysis.completed and
+          analysis.result and
           _DoAnalysesMatch(original_analysis, analysis) and
-          original_analysis.key is not analysis.key and
-          analysis.completed]
+          original_analysis.key is not analysis.key]
 
 
 def _TriageDuplicateResults(original_analysis, is_correct, user_name=None):
   matching_analyses = _GetDuplicateAnalyses(original_analysis)
 
   for analysis in matching_analyses:
-    _AppendTriageHistoryRecord(analysis, is_correct, user_name)
+    analysis.triage_reference_analysis_master_name = (
+        original_analysis.master_name)
+    analysis.triage_reference_analysis_builder_name = (
+        original_analysis.builder_name)
+    analysis.triage_reference_analysis_build_number = (
+        original_analysis.build_number)
+    _AppendTriageHistoryRecord(analysis, is_correct, user_name,
+                               is_duplicate=True)
+
+  return len(matching_analyses)

[lijeffrey, 2016/06/27 20:38:22]

It looks like you're returning the number of matching analyses that were found.
I would rename your method to something like _GetTriageDuplicateResultsCount or
something to make it more clear that this method returns a count and not just
performs actions

[josiahk, 2016/06/28 22:59:23]

Renamed to _TriageAndCountDuplicateResults()

 
 
 class TriageAnalysis(BaseHandler):
   PERMISSION_LEVEL = Permission.CORP_USER
 
   def HandleGet(self):  # pragma: no cover
     return self.HandlePost()
 
   def HandlePost(self):
     """Sets the manual triage result for the analysis.
 
     Mark the analysis result as correct/wrong/etc.
     TODO: make it possible to set the real culprit CLs.
     """
     url = self.request.get('url').strip()
     build_info = buildbot.ParseBuildUrl(url)
     if not build_info:
       return {'data': {'success': False}}
     master_name, builder_name, build_number = build_info
 
     is_correct = self.request.get('correct').lower() == 'true'
     # As the permission level is CORP_USER, we could assume the current user
     # already logged in.
     user_name = users.get_current_user().email().split('@')[0]
     success, original_analysis = _UpdateAnalysisResultStatus(
         master_name, builder_name, build_number, is_correct, user_name)
+    num_duplicate_analyses = 0
     if success:
-      _TriageDuplicateResults(original_analysis, is_correct, user_name)
-    return {'data': {'success': success}}
+      num_duplicate_analyses = _TriageDuplicateResults(
+          original_analysis, is_correct, user_name)
+    return {'data': {'success': success,
+                     'num_duplicate_analyses': num_duplicate_analyses}}