[Findit] Modify tryjob pipelines to trigger try jobs for test failure.

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 873db7b33193b2f654898b808481ec07e649e6ce..80de2e3e8e9031e37a0f8e2a027ebdf78c8a3892 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -9,45 +9,219 @@ from model.wf_try_job import WfTryJob
 from pipeline_wrapper import BasePipeline
 
 
+GIT_REPO = GitRepository(
+    'https://chromium.googlesource.com/chromium/src.git', HttpClient())

[stgao, 2016/01/26 00:51:40]

It seems in quite a few places in the code base, we use this url for quite a few
times.

We'd better reduce the occurrence of the same strings.
How about filing a bug to track this and fixing it in a separate CL?

[chanli, 2016/01/27 18:49:55]

Acknowledged.

+
+
 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 = {}
+    for failed_revision in failed_revisions:
+      change_log = GIT_REPO.GetChangeLog(failed_revision)
+      if change_log:
+        culprits[failed_revision] = {
+            'revision': failed_revision,
+            'commit_position': change_log.commit_position,
+            'review_url': change_log.code_review_url
+        }
+    return culprits
+
+  def _FindCulpritForEachTestFailure(self, blame_list, result):
+    # For test failures, the try job will run against every revision,

[stgao, 2016/01/26 00:51:40]

Just as a side note: This is the case for now. But we'd like to improve the
recipe findit/chromium/test and make it do the same as the
findit/chromium/compile -- stop testing a test if the first failure is found. In
this way, we could simplify the logic on Findit app side.

[chanli, 2016/01/27 18:49:55]

Acknowledged. I'll keep it in mind and make the change when the recipe is ready.

+    # so we need to traverse the result dict in order to identify the
+    # culprits to each failed step or test.
+    culprit_map = {}
+    failed_revisions = []
+    for revision in blame_list:
+      for step, step_result in result['result'][revision].iteritems():

[stgao, 2016/01/26 00:51:40]

What if the revision is not tested yet due to infra failure, like exception
during the recipe run?

[chanli, 2016/01/27 18:49:55]

If there were any infra failure or something else, data['result'] should be
'FAILURE' and there would be data['failure_reason'] as well, right?

I'll modify buildbucket_client a little to include those info. Then I'll add a
check on data['result'] == 'SUCCESS' and I'll handle other cases in a separate
CL.

+        if step_result['valid'] and step_result['status'] == 'failed':
+          if revision not in failed_revisions:
+            failed_revisions.append(revision)
+
+          if step not in culprit_map:
+            culprit_map[step] = {}
+            culprit_map[step]['suspected_cls'] = {
+                revision: {}
+            }
+            culprit_map[step]['suspected_cls'][revision]['revision'] = revision
+            culprit_map[step]['tests'] = {}
+
+          # Gets first failed revision for each test.
+          for failed_test in step_result['failures']:
+            if failed_test not in culprit_map[step]['tests']:
+              culprit_map[step]['tests'][failed_test] = {}

[stgao, 2016/01/26 00:51:40]

Under the failed_test, is there a reason to have another level {'revision':
revision}? Can it be just the string ``revision``?

[chanli, 2016/01/27 18:49:55]

Just like the example below, we will record the culprit info for each test as
well:
                'tests': {
                    'a_test1': {
                      'revision': 'rev1',
                      'commit_position': '1',
                      'review_url': 'url_1'
                    },
                    'a_test2': {
                      'revision': 'rev2',
                      'commit_position': '2',
                      'review_url': 'url_2'
                    }

+              culprit_map[step]['tests'][failed_test]['revision'] = (
+                  revision)
+              if revision not in culprit_map[step]['suspected_cls']:
+                # Different tests within the same step fail in different
+                # revisions, all revisions should be culprits for the step.
+                culprit_map[step]['suspected_cls'][revision] = {}
+                culprit_map[step]['suspected_cls'][revision]['revision'] = (
+                    revision)
+    return culprit_map, failed_revisions
+
+  def _UpdateCulpritMapWithCulpritInfo(self, culprit_map, culprits):
+    """Fills in commit_position and review_url for each failed rev in map."""
+    for step_culprit in culprit_map.values():
+      for revision, culprit_info in step_culprit['suspected_cls'].iteritems():
+        culprit_info.update(culprits[revision])
+      for test_culprit in step_culprit.get('tests', {}).values():
+        test_revision = test_culprit['revision']
+        test_culprit.update(culprits[test_revision])
+
   # Arguments number differs from overridden method - pylint: disable=W0221
   def run(
-      self, master_name, builder_name, build_number, try_job_id,
-      compile_result):
-    culprit = None
-
-    if compile_result and len(compile_result.get('result', [])) > 0:
-      # For compile failures, the try job will stop if one revision fails, so
-      # the culprit will be the last revision in the result.
-      result_for_last_checked_revision = compile_result['result'][-1]
-      failed_revision = (
-          result_for_last_checked_revision[0] if
-          result_for_last_checked_revision[1].lower() == 'failed' else None)
-
-      if failed_revision:
-        git_repo = GitRepository(
-            'https://chromium.googlesource.com/chromium/src.git', HttpClient())
-        change_log = git_repo.GetChangeLog(failed_revision)
-        if change_log:
-          culprit = {
-              'revision': failed_revision,
-              'commit_position': change_log.commit_position,
-              'review_url': change_log.code_review_url
-          }
-          compile_result['culprit'] = culprit
+      self, master_name, builder_name, build_number, blame_list, try_job_type,
+      try_job_id, result):
+    """Identifies the information for failed revisions.
+
+    The format for final try-job result for compile failures is:
+    {
+        'result': [
+            ['rev1', 'passed'],
+            ['rev2', 'failed']
+        ],
+        'url': 'url',
+        'try_job_id': '1',
+        'culprit': {
+            'revision': 'rev2',
+            'commit_position': '2',
+            'review_url': 'url_2'
+        }
+    }
+
+    The format for final try-job result for test failures is:

[stgao, 2016/01/26 00:51:40]

What do you think of adding a file recipe_result_format.md to explain the format
of the recipe results and referring to the file here instead?

[chanli, 2016/01/27 18:49:55]

Done.

+    {
+        'result': {
+            'rev1': {
+                'a_test': {
+                    'status': 'failed',
+                    'valid': True,
+                    'failures': ['a_test1']
+                },
+                'b_test': {
+                    'status': 'failed',
+                    'valid': True,
+                    'failures': ['b_test1']
+                },
+                'c_test': {
+                    'status': 'passed',
+                    'valid': True
+                }
+            },
+            'rev2': {
+                'a_test': {
+                    'status': 'failed',
+                    'valid': True,
+                    'failures': ['a_test1', 'a_test2']
+                },
+                'b_test': {
+                    'status': 'passed',
+                    'valid': True
+                },
+                'c_test': {
+                    'status': 'failed',
+                    'valid': True,
+                    'failures': []
+                }
+            }
+        },
+        'url': 'url',
+        'try_job_id': '1',
+        'culprit': {
+            'a_test': {
+                'suspected_cls': {
+                    'rev1': {
+                        'revision': 'rev1',
+                        'commit_position': '1',
+                        'review_url': 'url_1'
+                    },
+                    'rev2': {
+                        'revision': 'rev2',
+                        'commit_position': '2',
+                        'review_url': 'url_2'
+                    }
+                },
+                'tests': {
+                    'a_test1': {
+                      'revision': 'rev1',
+                      'commit_position': '1',
+                      'review_url': 'url_1'
+                    },
+                    'a_test2': {
+                      'revision': 'rev2',
+                      'commit_position': '2',
+                      'review_url': 'url_2'
+                    }
+                }
+            },
+            'b_test': {
+                'suspected_cls': {
+                    'rev1': {
+                        'revision': 'rev1',
+                        'commit_position': '1',
+                        'review_url': 'url_1'
+                    }
+                },
+                'tests': {
+                    'b_test1': {
+                      'revision': 'rev1',
+                      'commit_position': '1',
+                      'review_url': 'url_1'
+                    }
+                }
+            },
+            'c_test': {
+                'suspected_cls': {
+                    'rev2': {
+                      'revision': 'rev2',
+                      'commit_position': '2',
+                      'review_url': 'url_2',
+                    }
+                },
+                'tests': {}
+            }
+        }
+    }
+    """
+    culprits = None
+
+    if result and result.get('result'):
+      if try_job_type == 'compile':
+        # For compile failures, the try job will stop if one revision fails, so
+        # the culprit will be the last revision in the result.
+        result_for_last_checked_revision = result['result'][-1]
+        failed_revisions = (
+            [result_for_last_checked_revision[0]] if
+            result_for_last_checked_revision[1].lower() == 'failed' else [])
+
+        culprits = self._GetCulpritInfo(failed_revisions)
+        if culprits:
+          result['culprit'] = culprits[failed_revisions[0]]

[stgao, 2016/01/26 00:51:40]

Unrelated to this CL: In the else case, it means the try job can't identify the
culprit for some reason, i.e. infra exception.
Should we show a warming on the UI too?

[chanli, 2016/01/27 18:49:55]

Acknowledged. I'll file a bug for this as well.

+      else:  # try_job_type is 'test'.
+        culprit_map, failed_revisions = self._FindCulpritForEachTestFailure(
+            blame_list, result)
+        culprits = self._GetCulpritInfo(failed_revisions)
+        if culprits:
+          self._UpdateCulpritMapWithCulpritInfo(culprit_map, culprits)
+          result['culprit'] = culprit_map
 
     # Store try job results.
     try_job_result = WfTryJob.Get(master_name, builder_name, build_number)
-    if culprit:
-      if (try_job_result.compile_results and
-          try_job_result.compile_results[-1]['try_job_id'] == try_job_id):
-        try_job_result.compile_results[-1].update(compile_result)
+    if culprits:
+      result_needs_update = (

[stgao, 2016/01/26 00:51:40]

nit: naming.

[chanli, 2016/01/27 18:49:55]

Done.

+          try_job_result.compile_results if
+          try_job_type == 'compile' else try_job_result.test_results)
+      if (result_needs_update and
+          result_needs_update[-1]['try_job_id'] == try_job_id):
+        result_needs_update[-1].update(result)
       else:  # pragma: no cover
-        try_job_result.compile_results.append(compile_result)
+        result_needs_update.append(result)
 
     try_job_result.status = wf_analysis_status.ANALYZED
     try_job_result.put()
 
-    return culprit
+    return result.get('culprit', None) if result else None