[Sheriff-o-matic] Use likely_revisions instead of first_failing/last_passing

Tools/GardeningServer/ui/ct-failure-analyzer.html

Index: Tools/GardeningServer/ui/ct-failure-analyzer.html
diff --git a/Tools/GardeningServer/ui/ct-failure-analyzer.html b/Tools/GardeningServer/ui/ct-failure-analyzer.html
index 6add6dbcb15c47f896dd72073ffbd70747809292..a7cc596ae165ad27ff2a54f14bdb19c1a08f9136 100644
--- a/Tools/GardeningServer/ui/ct-failure-analyzer.html
+++ b/Tools/GardeningServer/ui/ct-failure-analyzer.html
@@ -35,34 +35,59 @@ found in the LICENSE file.
         ],
       },
 
-      _failureListComparator: function(tree, a, b) {
+      _failureByTreeListComparator: function(tree, a, b) {
         if (tree === undefined)
           tree = 'chromium';
 
-        var rev_a = a.failures[0].firstFailingRevisions;
-        var rev_b = b.failures[0].firstFailingRevisions;
-
-        // Handle missing revision.
-        if (!rev_a) {
-          if (!rev_b) {
+        if (!a.revisions || !a.revisions.length) {
+          if (!a.revisions || !b.revisions.length)
             return 0;
-          }
           return -1;
-        } else if (!rev_b) {
+        } else if (!b.revisions || !b.revisions.length) {
           return 1;
         }
 
-        // Prioritize the tree's revision.
-        if (rev_a[tree] != rev_b[tree])
-          return rev_b[tree] - rev_a[tree];
-
-        // Compare other revisions in alphabetical order.
-        var keys = Object.keys(rev_a).sort();
-        for (var i = 0; i < keys.length; i++) {
-          if (rev_a[keys[i]] != rev_b[keys[i]])
-            return rev_b[keys[i]] - rev_a[keys[i]];
+        // At this point |a.revisions| and |b.revisions| are sorted lists of
+        // revisions of the form '<tree>:<revision>', where <tree> is not
+        // necessarily |tree|. Get the latest revision, and favor those related
+        // to |tree|.
+        // FIXME: Sort by timestamp of the last (or first?) commit.
+        var _getLatestRevisionForTree = function(revisions) {
+          var i = 0;
+          var rev = revisions[revisions.length - 1 - i];

[ojan, 2014/08/25 18:09:40]

Nit: I'd find this easier to read if you started with i = revisions.length - 1
and just decremented i. It'd be more clear that this is walking backwards
through the array.

Also, what happens right now when i == revisions.length? I don't see a check for
that. I think you'd throw an exception...which the Promise would promptly
silently swallow. :( We're working on fixing the latter.

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

+          while (!rev.startsWith(tree)) {

[ojan, 2014/08/25 18:09:40]

Nit: Lets check startsWith(tree + ':') to avoid issues where one tree name is a
prefix of the other, e.g. I could easily imagine having v8 and v8-experimental
or something like that.
var treeWithColon = tree + ':';
while (!rev.startsWith(treeWithColon)) {

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

+            i++;
+            rev = revisions[revisions.length - 1 - i];
+          }
+          return rev;
+        };
+        var rev_a = _getLatestRevisionForTree(a.revisions, tree);
+        var rev_b = _getLatestRevisionForTree(b.revisions, tree);
+
+        // Extract the <tree> and <revision> parts.
+        var a_split = rev_a.split(':');
+        var a_tree = a_split[0];
+        var a_id = a_split[1];
+
+        var b_split = rev_b.split(':');
+        var b_tree = b_split[0];
+        var b_id = b_split[1];
+
+        // If trees mismatch, favor the one matching with |tree|. If none equal
+        // |tree|, b is favored.
+        if (a_tree != b_tree)
+          return b_tree == tree ? 1 : -1;
+
+        if (rev_a == rev_b) {

[ojan, 2014/08/25 18:09:40]

Can we move this check right after line 65? I think it'd make the code easier to
follow since all the splitting and ids and what not isn't needed in this block.

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

+          // In case of tree-revision equality, try a fallback.

[ojan, 2014/08/25 18:09:40]

This comment is a little confusing. Would be more clear if it said what the
fallback was. Or just leave the comment out.

More importantly, I don't think the fallback you have is right. What if the last
revisions are from different trees?

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

+          var a_last = a.revisions.last();
+          var b_last = b.revisions.last();
+          if (a_last == b_last)
+            return 0;
+          return a_last < b_last ? 1 : -1;
         }
-        return 0;
+
+        return b_id == a_id ? 0 : (b_id > a_id) ? 1 : -1;

[ojan, 2014/08/25 18:09:41]

Can this just be the following?
return b_id - a_id;

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

[Mathieu, 2014/08/26 15:47:40]

Acknowledged.

       },
 
       update: function() {
@@ -73,11 +98,13 @@ found in the LICENSE file.
             this.builderLatestRevisions = new CTBuilderRevisions(data.latest_builder_info['chromium.webkit']);
             this.failures = {};
             this.lastUpdateDate = new Date(data.date * 1000);
-            data.range_groups.forEach(function(group) {
-              this._processFailuresForGroup(group, data.alerts, annotations);
+            // Update |failures| with the appropriate CTFailureGroup's for each tree.
+            data.range_groups.forEach(function(rangeGroup) {
+              this._processFailuresForRangeGroup(rangeGroup, data.alerts, annotations);
             }.bind(this));
+            // Sort failure groups so that newer failures are shown at the top of the UI.
             Object.keys(this.failures, function (tree, failuresByTree) {
-              this.failures[tree].sort(this._failureListComparator.bind(this, tree));
+              this.failures[tree].sort(this._failureByTreeListComparator.bind(this, tree));
             }.bind(this));
           }.bind(this));
         }.bind(this));
@@ -95,9 +122,7 @@ found in the LICENSE file.
         return 0;
       },
 
-      _processFailuresForGroup: function(group, failures, annotations) {
-        var failuresByReason = {};
-
+      _processFailuresForRangeGroup: function(rangeGroup, alerts, annotations) {
         var masterToTree = {};
         Object.keys(this._trees, function(tree, masters) {
           masters.forEach(function(master) {
@@ -105,8 +130,13 @@ found in the LICENSE file.
           });
         });
 
-        group.failure_keys.forEach(function(failure_key) {
-          var failure = failures.find(function(item) { return item.key == failure_key; });
+        // 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; });
+          // 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.
@@ -123,6 +153,8 @@ found in the LICENSE file.
             reason: 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.
@@ -140,13 +172,17 @@ found in the LICENSE file.
           };
         }.bind(this));
 
+        if (!Object.keys(failuresByReason).length)
+          return;
+
         var groupedFailures = {};
         Object.keys(failuresByReason, function(failureKey, resultsByTree) {
           var failure = JSON.parse(failureKey);
           Object.keys(resultsByTree, function(tree, resultsByBuilder) {
             if (!groupedFailures[tree])
               groupedFailures[tree] = [];
-            groupedFailures[tree].push(new CTFailure(failure.step, failure.reason, resultsByBuilder, group.merged_first_failing, group.merged_last_passing));
+            groupedFailures[tree].push(
+                new CTFailure(failure.step, failure.reason, resultsByBuilder));
           })
         });
 
@@ -156,8 +192,10 @@ found in the LICENSE file.
           if (!this.failures[tree])
             this.failures[tree] = [];
           // FIXME: Need a better identifier for a failure group;
-          var key = group.sort_key;
-          this.failures[tree].push(new CTFailureGroup(key, failures, annotations[key]));
+          var key = rangeGroup.sort_key;
+          this.failures[tree].push(
+              new CTFailureGroup(key, failures, rangeGroup.likely_revisions,
+                                 annotations[key]));
         }.bind(this));
       },
     });