[Findit] Adding Try Job suspected CLs and result status to analysis

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 3a270c9141714d25952ae891344dc3ded4ac33ef..b31e093e8475439c335509ae9bba1dcb5fc611a5 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -4,7 +4,9 @@
 
 from common.git_repository import GitRepository
 from common.http_client_appengine import HttpClientAppengine as HttpClient
+from model import wf_analysis_result_status

[stgao, 2016/04/11 19:50:12]

you may want to rebase on the refactoring CL I committed.

 from model import wf_analysis_status
+from model.wf_analysis import WfAnalysis
 from model.wf_try_job import WfTryJob
 from model.wf_try_job_data import WfTryJobData
 from pipeline_wrapper import BasePipeline
@@ -15,15 +17,92 @@ GIT_REPO = GitRepository(
     'https://chromium.googlesource.com/chromium/src.git', HttpClient())
 
 
+def _GetResultAnalysisStatus(analysis, result):
+  """Returns the analysis status based on existing status and try job result.
+
+  Args:
+    analysis: The WfAnalysis entity corresponding to this try job.
+    result: A result dict containing the result of this try job.
+
+  Returns:
+    A wf_analysis_result_status code.
+  """
+  # Only return an updated analysis result status if no results were already
+  # found (by the heuristic-based approach). Note it is possible the
+  # heuristic-based result was triaged before the completion of this try job.
+  analysis_result_status = analysis.result_status
+
+  if (analysis_result_status is None or
+      analysis_result_status == wf_analysis_result_status.NOT_FOUND_UNTRIAGED or
+      analysis_result_status == wf_analysis_result_status.NOT_FOUND_INCORRECT or
+      analysis_result_status == wf_analysis_result_status.NOT_FOUND_CORRECT):
+    if result and result.get('culprit'):
+      return wf_analysis_result_status.FOUND_UNTRIAGED
+    return wf_analysis_result_status.NOT_FOUND_UNTRIAGED

[stgao, 2016/04/11 19:50:12]

Why we reset wf_analysis_result_status.NOT_FOUND_INCORRECT and
wf_analysis_result_status.NOT_FOUND_CORRECT to
wf_analysis_result_status.NOT_FOUND_UNTRIAGED if the try-job find nothing?

[lijeffrey, 2016/04/12 21:10:54]

Done.

+
+  return analysis_result_status
+
+
+def _GetSuspectedCLs(analysis, result):
+  """Returns a list of suspected CLs.
+
+  Args:
+    analysis: The WfAnalysis entity corresponding to this try job.
+    result: A result dict containing the culprit from the results of
+      this tryjob.
+
+  Returns:
+    A combined list of suspected CLs from those already in analysis and those
+    found by this try job.
+  """
+  suspected_cls = analysis.suspected_cls or []

[stgao, 2016/04/11 19:50:13]

Should we do a copy of analysis.suspected_cls instead of updating it directly
through a reference?

[lijeffrey, 2016/04/12 21:10:54]

Done.

+
+  if not result:
+    return suspected_cls
+
+  culprit = result.get('culprit')
+  if not culprit:
+    return suspected_cls
+
+  compile_cl_info = culprit.get('compile')
+  if compile_cl_info:
+    # Suspected CL is from compile failure.
+    if compile_cl_info not in suspected_cls:
+      suspected_cls.append(compile_cl_info)
+      return suspected_cls

[stgao, 2016/04/11 19:50:12]

How about the "else" case of line #70?

[lijeffrey, 2016/04/12 21:10:54]

Done.

+
+  # Suspected CLs are from test failures.
+  for results in culprit.itervalues():
+    if results.get('revision'):
+      # Non swarming test failures, only have step level failure info.
+      cl_info = {
+          'review_url': results.get('review_url'),
+          'repo_name': results.get('repo_name'),
+          'revision': results.get('revision'),
+          'commit_position': results.get('commit_position')
+      }
+      if cl_info not in suspected_cls:
+        suspected_cls.append(cl_info)
+    else:
+      for test_cl_info in results['tests'].values():
+        if test_cl_info not in suspected_cls:
+          suspected_cls.append(test_cl_info)
+
+  return suspected_cls
+
+
 class IdentifyTryJobCulpritPipeline(BasePipeline):
   """A pipeline to identify culprit CL info based on try job compile results."""
 
   def _GetCulpritInfo(self, failed_revisions):
     """Gets commit_positions and review_urls for revisions."""
     culprits = {}
+    # TODO(lijeffrey): remove hard-coded 'chromium' when DEPS file parsing is
+    # supported.
     for failed_revision in failed_revisions:
       culprits[failed_revision] = {
-          'revision': failed_revision
+          'revision': failed_revision,
+          'repo_name': 'chromium'
       }
       change_log = GIT_REPO.GetChangeLog(failed_revision)
       if change_log:
@@ -106,6 +185,7 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
             not culprit_map[step].get('revision')):
           # Non swarming test failures, only have step level failure info.
           culprit_map[step]['revision'] = revision
+          culprit_map[step]['repo_name'] = 'chromium'
 
         for failed_test in test_result['failures']:
           # Swarming tests, gets first failed revision for each test.
@@ -188,4 +268,10 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
     try_job_result.status = wf_analysis_status.ANALYZED
     try_job_result.put()
 
+    # Update analysis result and suspected CLs with results of this try job.
+    analysis = WfAnalysis.Get(master_name, builder_name, build_number)
+    analysis.result_status = _GetResultAnalysisStatus(analysis, result)

[stgao, 2016/04/11 19:50:13]

If result is empty, no checking is needed in which case we could avoid a
datastore read&write.

[lijeffrey, 2016/04/12 21:10:54]

Done.

+    analysis.suspected_cls = _GetSuspectedCLs(analysis, result)
+    analysis.put()
+
     return result.get('culprit') if result else None