[Findit] Change the criteria of sending notification to code review.

appengine/findit/waterfall/identify_try_job_culprit_pipeline.py

Index: appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
index 12be524fe7fe94d697e053b80b743b824dd5c3b3..599ec63cbd3992add93b486274c48b6965f78f4e 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -182,16 +182,56 @@ def _GetCulpritsForTestsFromResultsDict(blame_list, test_results):
   return culprit_map, list(failed_revisions)
 
 
-def _NotifyCulprits(master_name, builder_name, build_number, culprits):
-  """Sends notifications to the identified culprits."""
+def _GetSuspectedCLFoundByHeuristicForCompile(analysis):
+  """For compile failure, gets the suspected revision found by heuristic."""
+  if not analysis or not analysis.result:
+    return None
+  for failure in analysis.result.get('failures', []):
+    if (failure['step_name'].lower() == 'compile' and
+        len(failure['suspected_cls']) == 1):
+      # Based on confidence calculation, suspected_cl found by heuristic for
+      # compile is very likely to be the culprit.
+      # Since the current confidence calculation is for results with single
+      # suspected_cl, we might need to have the same regulation here.
+      return failure['suspected_cls'][0]
+  return None
+
+
+def _GetHeuristicSuspectedCLs(analysis):
+  """Gets revisions of suspected cls found by heuristic approach."""
+  if analysis and analysis.suspected_cls:
+    return [(cl['repo_name'], cl['revision']) for cl in analysis.suspected_cls]
+  return []
+
+
+def _StartSendNotificationPipeline(
+    master_name, builder_name, build_number, repo_name, revision):
   try:
-    for culprit in (culprits or {}).itervalues():
-      pipeline = SendNotificationForCulpritPipeline(
-          master_name, builder_name, build_number,
-          culprit['repo_name'], culprit['revision'])
-      pipeline.start()
+    pipeline = SendNotificationForCulpritPipeline(
+      master_name, builder_name, build_number, repo_name, revision)
+    pipeline.start()
   except Exception:  # pragma: no cover.
-    logging.exception('Failed to notify culprits.')
+    logging.exception(
+        'Failed to notify culprits which was found by both approaches.')
+
+
+def _NotifyCulprits(master_name, builder_name, build_number, culprits,
+                    heuristic_cls, compile_suspected_cl):
+  """Sends notifications to the identified culprits."""
+
+  if culprits:

[Sharu Jiang, 2016/09/02 18:00:22]

If culprits mean try_job_cls, I think rename it to try_job_cls would be more
clear.

[chanli, 2016/09/02 18:25:59]

It is a convention to call a suspected_cl found by try job as culprit because
try_job should be able to give us the culprit if no flakiness.

I would not change it here since we need to change a lot of places if we want to
make this change and keep everything in sync.

+    # There is try job result, we need to check if any of the culprit
+    # was also found by heuristic.
+    for culprit in culprits.itervalues():
+      if (culprit['repo_name'], culprit['revision']) in heuristic_cls:
+        _StartSendNotificationPipeline(
+            master_name, builder_name, build_number,
+            culprit['repo_name'], culprit['revision'])
+  elif compile_suspected_cl:
+    # Need to check if the failure is compile and heuristic found a culprit.
+    _StartSendNotificationPipeline(
+        master_name, builder_name, build_number,
+        compile_suspected_cl['repo_name'], compile_suspected_cl['revision'])
 
 
 class IdentifyTryJobCulpritPipeline(BasePipeline):
@@ -324,7 +364,6 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
 
       # Update analysis result and suspected CLs with results of this try job if
       # culprits were found.
-      analysis = WfAnalysis.Get(master_name, builder_name, build_number)
       updated_result_status = _GetResultAnalysisStatus(analysis, result)
       updated_suspected_cls = _GetSuspectedCLs(analysis, result)
 
@@ -334,10 +373,19 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
         analysis.suspected_cls = updated_suspected_cls
         analysis.put()
 
-    # Store try-job results.
+      # Store try-job results.
     UpdateTryJobResult()
+
+

[Sharu Jiang, 2016/09/02 18:00:22]

nit: delete one empty line.

[chanli, 2016/09/02 18:25:59]

Done.

+    analysis = WfAnalysis.Get(master_name, builder_name, build_number)
+    heuristic_cls = _GetHeuristicSuspectedCLs(analysis)
+    compile_suspected_cl = (
+        _GetSuspectedCLFoundByHeuristicForCompile(analysis)
+        if try_job_type == failure_type.COMPILE else None)
+
     # Add try-job results to WfAnalysis.
     UpdateWfAnalysisWithTryJobResult()
 
-    _NotifyCulprits(master_name, builder_name, build_number, culprits)
-    return result.get('culprit') if result else None
+    _NotifyCulprits(master_name, builder_name, build_number, culprits,
+                    heuristic_cls, compile_suspected_cl)
+    return result.get('culprit') if result else None