Refactor CTFailures._processFailuresForRangeGroup.

Tools/GardeningServer/model/ct-failures.html

 <!--
 Copyright 2014 The Chromium Authors. All rights reserved.
 Use of this source code is governed by a BSD-style license that can be
 found in the LICENSE file.
 -->
 
 <link rel="import" href="../lib/net.html">
 <link rel="import" href="../lib/update-util.html">
 <link rel="import" href="ct-builder-revisions.html">
 <link rel="import" href="ct-failure.html">
@@ skipping 20 matching lines @@
       "https://build.chromium.org/p/chromium.chromiumos",
       "https://build.chromium.org/p/chromium.gpu",
       "https://build.chromium.org/p/chromium.linux",
       "https://build.chromium.org/p/chromium.mac",
       "https://build.chromium.org/p/chromium.memory",
       "https://build.chromium.org/p/chromium.win"
     ],
   };
 }
 
+(function() {
+'use strict';
+
 // Reverse sorting order, if a > b, return a negative number.
 CTFailures.prototype._failureByTreeListComparator = function(tree, a, b) {
   if (tree === undefined)
     tree = 'chromium';
 
   var rev_a = a.commitList.revisions;
   var rev_b = b.commitList.revisions;
 
   if (!rev_a || !Object.keys(rev_a).length) {
     if (!rev_b || !Object.keys(rev_b).length)
@@ skipping 19 matching lines @@
     var b_list = rev_b[keys[i]];
     if (!b_list)
       return -1;
 
     if (a_list.last() != b_list.last())
       return b_list.last() - a_list.last();
   }
   return 0;
 };
 
+// Updates the format of the alerts array to be easier to parse.
+CTFailures.prototype._mungeAlerts = function(alerts) {

[ojan, 2014/09/05 00:56:42]

Mind writing basic unit tests for this function? I'm hoping to head in the
direction of 100% test coverage for at least the JS code in sheriff-o-matic.

[Jeffrey Yasskin, 2014/09/09 01:00:32]

Done.

+  var masterToTree = {};
+  Object.keys(this._trees, function(tree, masters) {
+    masters.forEach(function(master) {
+      masterToTree[master] = tree;
+    });
+  });
+
+  alerts.forEach(function(failure) {
+    // FIXME: Have the server store the actual failure type in a different
+    // field instead of smashing it into the reason.
+    if (failure.failureType) {
+      // The server has been fixed.

[ojan, 2014/09/05 00:56:42]

<3

+    } else {
+      if (failure.reason) {
+        var parts = failure.reason.split(':');
+        failure.reason = parts[0];
+        failure.failureType = parts[1] || 'FAIL';
+      } else {
+        failure.failureType = 'UNKNOWN';
+      }
+    }
+
+    // FIXME: Figure out what tree masters that aren't in masterToTree
+    // we should have.
+    failure.tree = masterToTree[failure.master_url];
+  });
+};
+
 CTFailures.prototype.update = function() {
   var annotationPromise = CTFailureGroup.fetchAnnotations();
 
   return net.json('http://sheriff-o-matic.appspot.com/alerts').then(function(data) {
     return annotationPromise.then(function(annotations) {
       // FIXME: Don't special-case the blink master.
       this.builderLatestRevisions = new CTBuilderRevisions(data.latest_builder_info['chromium.webkit']);
       var newFailures = {};
       this.lastUpdateDate = new Date(data.date * 1000);
+      this._mungeAlerts(data.alerts);
+      var alertsByKey = {}

[ojan, 2014/09/05 00:56:42]

Add a FIXME to change builder_alerts to just have the alerts be a map instead of
a list? Other than builder_alerts itself, this is the only consumer of this
field.

[Jeffrey Yasskin, 2014/09/09 01:00:33]

Done.

+      data.alerts.forEach(function(alert) {
+        alertsByKey[alert.key] = alert;
+      });
       // Update |failures| with the appropriate CTFailureGroup's for each
       // tree.
       data.range_groups.forEach(function(rangeGroup) {
-        this._processFailuresForRangeGroup(newFailures, rangeGroup, data.alerts, annotations);
+        this._processFailuresForRangeGroup(newFailures, rangeGroup, alertsByKey, annotations);
       }.bind(this));
       // Sort failure groups so that newer failures are shown at the top
       // of the UI.
       Object.keys(newFailures, function (tree, failuresByTree) {
         failuresByTree.sort(this._failureByTreeListComparator.bind(this, tree));
       }.bind(this));
       this.failures = updateUtil.updateLeft(this.failures, newFailures);
     }.bind(this));
   }.bind(this));
 };
 
 CTFailures.prototype._failureComparator = function(a, b) {
   if (a.step > b.step)
     return 1;
   if (a.step < b.step)
     return -1
   if (a.testName > b.testName)
     return 1;
   if (a.testName < b.testName)
     return -1
   return 0;
 };
 
-CTFailures.prototype._processFailuresForRangeGroup = function(newFailures, rangeGroup, alerts, annotations) {
-  var masterToTree = {};
-  Object.keys(this._trees, function(tree, masters) {
-    masters.forEach(function(master) {
-      masterToTree[master] = tree;
-    });
-  });
-
-  // A rangeGroup may be related to multiple alerts (via |failure_keys|). Categorize
-  // these failures by reason (cause of failure), so that they can be grouped in the UI
-  // (via a CTFailureGroup). Failures will be grouped in |failuresByReason|.
-  var failuresByReason = {};
-  rangeGroup.failure_keys.forEach(function(failure_key) {
-    var failure = alerts.find(function(item) { return item.key == failure_key; });
+function groupFailuresByTreeAndReason(failures, annotations) {

[ojan, 2014/09/05 00:56:42]

Ditto. You'll need to make this a function on CTFailures to expose it to the
test, but I think that's OK.

[Jeffrey Yasskin, 2014/09/09 01:00:32]

To do the refactoring that Elliott asked for in
https://codereview.chromium.org/526633002/, I think we'd need to push the loop
over data.range_groups down through _processFailuresForRangeGroup into this
function, and have this function return a map
"tree->group->reasonKey->builder->info".

I'm not planning to pursue this further.

[Jeffrey Yasskin, 2014/09/09 01:00:33]

Tested.

+  var failuresByTree = {};
+  failures.forEach(function(failure) {
     // Establish the key to uniquely identify a failure by reason.
-    var reason, failureType;
-    if (failure.reason) {
-      // FIXME: Store the actual failure type in a different field instead of smashing it into the reason.
-      var parts = failure.reason.split(':');
-      reason = parts[0];
-      failureType = parts[1] || 'FAIL';
-    } else {
-      reason = null;
-      failureType = 'UNKNOWN';
-    }
-
     var failureKey = CTFailure.createKey(failure);
 
     var reasonKey = JSON.stringify({
       step: failure.step_name,
-      reason: reason,
+      reason: failure.reason,
     });
 
-    // FIXME: Figure out what tree masters that aren't in masterToTree
-    // we should have.
-    var tree = masterToTree[failure.master_url];
-
     // FIXME: Use a model class instead of a dumb object.
-    if (!failuresByReason[reasonKey])
-      failuresByReason[reasonKey] = {};
-    if (!failuresByReason[reasonKey][tree])
-      failuresByReason[reasonKey][tree] = {};
-    failuresByReason[reasonKey][tree][failure.builder_name] = {
+    if (!failuresByTree[failure.tree])
+      failuresByTree[failure.tree] = {};
+    if (!failuresByTree[failure.tree][reasonKey])
+      failuresByTree[failure.tree][reasonKey] = {};
+    failuresByTree[failure.tree][reasonKey][failure.builder_name] = {
       key: failureKey,
       // FIXME: Rename to failureType.
-      actual: failureType,
+      actual: failure.failureType,
       lastFailingBuild: failure.last_failing_build,
       earliestFailingBuild: failure.failing_build,
       masterUrl: failure.master_url,
       failingBuildCount: failure.failing_build_count,
       annotation: annotations[failureKey],
     };
-  }.bind(this));
+  });
+  return failuresByTree
+};
 
-  if (!Object.keys(failuresByReason).length)
+CTFailures.prototype._processFailuresForRangeGroup = function(newFailures, rangeGroup, alerts, annotations) {
+  // A rangeGroup may be related to multiple alerts (via |failure_keys|). Categorize
+  // these failures by reason (cause of failure), so that they can be grouped in the UI
+  // (via a CTFailureGroup).
+  var failures = rangeGroup.failure_keys.map(function(failure_key) {
+    return alerts[failure_key];
+  });
+  var failuresByTree = groupFailuresByTreeAndReason(failures, annotations);
+
+  if (Object.isEmpty(failuresByTree))
     return;
 
-  // Maps a tree id to a list of CTFailures in that tree.
-  var groupedFailures = {};
-  Object.keys(failuresByReason, function(reasonKey, resultsByTree) {
-    var failure = JSON.parse(reasonKey);
-    Object.keys(resultsByTree, function(tree, resultsByBuilder) {
-      if (!groupedFailures[tree])
-        groupedFailures[tree] = [];
-      groupedFailures[tree].push(
+  Object.keys(failuresByTree, function(tree, resultsByReason) {
+    var treeFailures = [];
+    Object.keys(resultsByReason, function(reasonKey, resultsByBuilder) {
+      var failure = JSON.parse(reasonKey);
+      treeFailures.push(
           new CTFailure(failure.step, failure.reason, resultsByBuilder));
     })
-  });
-
-  Object.keys(groupedFailures, function(tree, failures) {
-    failures.sort(this._failureComparator);
+    treeFailures.sort(this._failureComparator);
 
     if (!newFailures[tree])
       newFailures[tree] = [];
     var commitList = new CTCommitList(this.commitLog, rangeGroup.likely_revisions);
-    newFailures[tree].push(new CTFailureGroup(rangeGroup.sort_key, failures, commitList));
+    newFailures[tree].push(new CTFailureGroup(rangeGroup.sort_key, treeFailures, commitList));
   }.bind(this));
 };
 
+})();
 </script>