[Findit] Flake Checker: Extract commit position and git hash and display to UI for each analyzed bu…

appengine/findit/handlers/flake/check_flake.py

Index: appengine/findit/handlers/flake/check_flake.py
diff --git a/appengine/findit/handlers/flake/check_flake.py b/appengine/findit/handlers/flake/check_flake.py
index 9363b5e383527abd89285a3060daa53738d8ad8c..1df837d29e32620619bd2adb9a4fcde7da0727cb 100644
--- a/appengine/findit/handlers/flake/check_flake.py
+++ b/appengine/findit/handlers/flake/check_flake.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import json
 import logging
 
 from google.appengine.api import users
@@ -11,10 +12,13 @@ from common import auth_util
 from common.base_handler import BaseHandler
 from common.base_handler import Permission
 from libs import time_util
+from libs.gitiles import commit_util
 from model import analysis_status
 from model import triage_status
 from model.flake.flake_analysis_request import FlakeAnalysisRequest
 from model.flake.master_flake_analysis import MasterFlakeAnalysis
+from waterfall import buildbot
+from waterfall import build_util
 from waterfall.flake import flake_analysis_service
 from waterfall.flake import triggering_sources
 
@@ -22,10 +26,63 @@ from waterfall.flake import triggering_sources
 SWARMING_TASK_BASE_URL = 'https://chromium-swarm.appspot.com'
 
 
-def _GetSuspectedFlakeAnalysisAndTriageResult(analysis):
+def _GetCommitPositionAndGitHash(master_name, builder_name, build_number):
+  """Gets the commit position and git hash of a build.
+
+  Args:
+    master_name (str): The name of the master.
+    builder_name (str): The name of the builder.
+    build_number (int): The build number.
+
+  Returns:
+    commit_position (int), git_hash (str): The git commit position corresponding
+      to the last commit in the build, and the git hash itself.
+  """
+  build = build_util.DownloadBuildData(master_name, builder_name, build_number)

[stgao, 2016/12/12 20:05:18]

This seems not good. What if there are 100s of data points?
In that case, too many HTTP requests have to be sent.

Instead of retrieving info during page rendering, we'd better retrieve at some
stage of the Swarming rerun pipeline, save the data in NDB together with the
DataPoint, and then here we just read them directly.

[lijeffrey, 2016/12/14 02:37:13]

Done.

+
+  if not build.data:
+    return None, None
+
+  build_data = json.loads(build.data)
+  git_hash = buildbot.GetBuildProperty(
+      build_data.get('properties', []), 'got_revision')
+  commit_position = None
+  changes = build_data.get('sourceStamp', {}).get('changes', [])
+
+  if changes:  # pragma: no branch
+    last_commit_message = changes[-1].get('comments')
+    commit_position, _ = commit_util.ExtractCommitPositionAndCodeReviewUrl(
+        last_commit_message)
+
+  return commit_position, git_hash
+
+
+def _GetSuspectedFlakeInfo(analysis):
+  """Returns a dict with information about the suspected flake build.
+
+  Args:
+    analysis (MasterFlakeAnalysis): The master flake analysis the suspected
+      flake build is associated with.
+
+  Returns:
+    A dict in the format:
+      {
+          'build_number': int,
+          'triage_result': int (correct, incorrect, etc.)
+          'commit_position': The human-readable commit position associated with
+            the suspected flaky build number.
+          'git_hash': The git hash associated with the suspected flaky build.
+      }
+  """
+
   if analysis.suspected_flake_build_number is not None:
+    commit_position, git_hash = _GetCommitPositionAndGitHash(
+        analysis.master_name, analysis.builder_name, analysis.build_number)
+
     return {
         'build_number': analysis.suspected_flake_build_number,
+        'commit_position': commit_position,
+        'git_hash': git_hash,
         'triage_result': (
             analysis.triage_history[-1].triage_result if analysis.triage_history
             else triage_status.UNTRIAGED)
@@ -34,6 +91,29 @@ def _GetSuspectedFlakeAnalysisAndTriageResult(analysis):
   return {}
 
 
+def _GetCoordinatesData(analysis):
+  if not analysis or not analysis.data_points:
+    return []
+
+  coordinates = []
+
+  for data_point in analysis.data_points:
+    # Get commit position and git hash
+    commit_position, git_hash = _GetCommitPositionAndGitHash(
+        analysis.master_name, analysis.builder_name, data_point.build_number)
+    task_url = ('%s/task?id=%s' % (
+        SWARMING_TASK_BASE_URL, data_point.task_id) if data_point.task_id
+                else None)

[chanli, 2016/12/12 14:19:05]

Nit: indent

[lijeffrey, 2016/12/14 02:37:13]

Done.

+    coordinates.append([
+        data_point.build_number, data_point.pass_rate, task_url,
+        commit_position, git_hash])
+
+  # Order by build number from earliest to latest.
+  coordinates.sort(key=lambda x: x[0])
+
+  return coordinates
+
+
 class CheckFlake(BaseHandler):
   PERMISSION_LEVEL = Permission.ANYONE
 
@@ -152,7 +232,7 @@ class CheckFlake(BaseHandler):
               'return_code': 400
           }
 
-    suspected_flake = _GetSuspectedFlakeAnalysisAndTriageResult(analysis)
+    suspected_flake = _GetSuspectedFlakeInfo(analysis)
 
     data = {
         'key': analysis.key.urlsafe(),
@@ -185,21 +265,8 @@ class CheckFlake(BaseHandler):
           analysis.start_time,
           analysis.end_time or time_util.GetUTCNow())
 
-    coordinates = []
-    for data_point in analysis.data_points:
-      if data_point.task_id:
-        task_url = '%s/task?id=%s' % (
-            SWARMING_TASK_BASE_URL, data_point.task_id)
-        coordinates.append([
-            data_point.build_number, data_point.pass_rate, task_url])
-      else:
-        coordinates.append([
-            data_point.build_number, data_point.pass_rate, None])
-
-    # Order by build number from earliest to latest.
-    coordinates.sort(key=lambda x: x[0])
-
-    data['pass_rates'] = coordinates
+    data['pass_rates'] = _GetCoordinatesData(analysis)
+
     return {
         'template': 'flake/result.html',
         'data': data