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

appengine/findit/waterfall/send_notification_for_culprit_pipeline.py

Index: appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
index 5f40f4a3996ffae07e88c67d2ea99d88d7b3f3bc..d854103d0f1d72565cfc8ecd9b18b3bd2ba63cd8 100644
--- a/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
+++ b/appengine/findit/waterfall/send_notification_for_culprit_pipeline.py
@@ -14,15 +14,25 @@ from common.http_client_appengine import HttpClientAppengine as HttpClient
 from common.pipeline_wrapper import BasePipeline
 from common.rietveld import Rietveld
 from model import analysis_status as status
+from model.wf_analysis import WfAnalysis
 from model.wf_culprit import WfCulprit
 from waterfall import build_util
 from waterfall import waterfall_config
 
 
+def _PassAdditionalCriteria(additional_criteria):

[lijeffrey1, 2016/09/06 16:53:56]

nit: rename this _AdditionalCriteriaAllPassed(additional_criteria)

[chanli, 2016/09/06 17:12:42]

Done.

+  """Check if the all the additional criteria have passed.
+
+  Checks for individual criterion have been done before this function.
+  """
+  return all(additional_criteria.values())
+
+
 @ndb.transactional
 def _ShouldSendNotification(
     master_name, builder_name, build_number, repo_name, revision,
-    commit_position, build_num_threshold, time_limit_passed):
+    commit_position, build_num_threshold, additional_criteria,
+    send_notification_right_now):
   """Returns True if a notification for the culprit should be sent."""
   culprit = (WfCulprit.Get(repo_name, revision) or
              WfCulprit.Create(repo_name, revision, commit_position))
@@ -38,8 +48,10 @@ def _ShouldSendNotification(
   #    * Try-job takes too long to complete and the failure got fixed.
   #    * The whole analysis was rerun a long time after the failure occurred.
   should_send = not (culprit.cr_notification_processed or
-                     len(culprit.builds) < build_num_threshold or
-                     time_limit_passed)
+                     not _PassAdditionalCriteria(additional_criteria))
+  if not send_notification_right_now:
+    should_send = should_send and len(culprit.builds) >= build_num_threshold
+
   if should_send:
     culprit.cr_notification_status = status.RUNNING
   culprit.put()
@@ -63,10 +75,10 @@ def _SendNotificationForCulprit(
     culprit = WfCulprit.Get(repo_name, revision)
     rietveld = Rietveld()
     message = textwrap.dedent("""
-    FYI: Findit try jobs (rerunning failed compile or tests) identified this CL
-    at revision %s as the culprit for failures in the build cycles as shown on:
-    https://findit-for-me.appspot.com/waterfall/culprit?key=%s
-    """) % (commit_position or revision, culprit.key.urlsafe())
+    FYI: Findit identified this CL at revision %s as the culprit for
+    failures in the build cycles as shown on:
+    https://findit-for-me.appspot.com/waterfall/culprit?key=%s""") % (
+        commit_position or revision, culprit.key.urlsafe())
     sent = rietveld.PostMessage(code_review_url, message)
   else:
     logging.error('No code-review url for %s/%s', repo_name, revision)
@@ -86,19 +98,21 @@ def _GetCulpritInfo(repo_name, revision):
   return change_log.commit_position, change_log.code_review_url
 
 
-def _NotificationTimeLimitPassed(build_end_time, latency_limit_minutes):
-  """Returns True if it is too late to send notification."""
+def _WithinNotificationTimeLimit(build_end_time, latency_limit_minutes):
+  """Returns True if it is still in time to send notification."""
   latency_seconds = (time_util.GetUTCNow() - build_end_time).total_seconds()
-  return latency_seconds > latency_limit_minutes * 60
+  return latency_seconds <= latency_limit_minutes * 60
 
 
 class SendNotificationForCulpritPipeline(BasePipeline):
 
   # Arguments number differs from overridden method - pylint: disable=W0221
-  def run(self, master_name, builder_name, build_number, repo_name, revision):
+  def run(
+      self, master_name, builder_name, build_number, repo_name, revision,
+      send_notification_right_now):
     action_settings = waterfall_config.GetActionSettings()
     # Set some impossible default values to prevent notification by default.
-    build_threshold = action_settings.get(
+    build_num_threshold = action_settings.get(
         'cr_notification_build_threshold', 100000)
     latency_limit_minutes = action_settings.get(
         'cr_notification_latency_limit_minutes', 1)
@@ -107,12 +121,21 @@ class SendNotificationForCulpritPipeline(BasePipeline):
         repo_name, revision)
     build_end_time = build_util.GetBuildEndTime(
         master_name, builder_name, build_number)
-    time_limit_passed = _NotificationTimeLimitPassed(
+    within_time_limit = _WithinNotificationTimeLimit(
         build_end_time, latency_limit_minutes)
 
+    # Additional criteria that will help decide if a notification
+    # should be sent.
+    # TODO (chanli): Add check for if confidence for the culprit is
+    # over threshold.
+    additional_criteria = {
+        'within_time_limit': within_time_limit
+    }
+
     if not _ShouldSendNotification(
       master_name, builder_name, build_number, repo_name,
-      revision, commit_position, build_threshold, time_limit_passed):
+      revision, commit_position, build_num_threshold, additional_criteria,
+      send_notification_right_now):
       return False
     return _SendNotificationForCulprit(
         repo_name, revision, commit_position, code_review_url)