[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..0d0996796d774d10f0ddb3f2697bbd67e599e160 100644
--- a/Tools/GardeningServer/ui/ct-failure-analyzer.html
+++ b/Tools/GardeningServer/ui/ct-failure-analyzer.html
@@ -35,34 +35,58 @@ 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

[ojan, 2014/08/23 01:23:06]

It turns out that we'll be able to keep using git commit positions (the
equivalent of svn revisions) at least for the short-term. So, we should be able
to keep this mostly the same as how it is today. Will just need to change how we
get the revision numbers above. Sorry for the churn. I didn't realize that we'd
be able to use git commit positions till I talked to stip today.

Down the road, we should still change this code to be git hash friendly, but the
way I want to do that is to expose timestamps in the JSON from the server, so
the logic here would stay the same still, it would just use timestamps instead
of revision numbers.

[Mathieu, 2014/08/24 23:42:11]

No problem. Are you fine with this implementation then? It's not perfect, but it
works well for hashes and numbers.

[ojan, 2014/08/25 03:54:13]

I'd rather use the old implementation. It's considerably simpler since it only
needs to handle numbers. I don't think we'll ever try to make this work with
hashes since we'd just use timestamps if we wanted to move off commit positions.

[Mathieu, 2014/08/25 13:03:22]

Much of my rewrite was because the old arguments were objects and now we have a
list e.g. ['chromium:1', 'blink:2']. Not use how to bring it back to the old
impl and still use |revisions|. Let's chat on what you mean later.

+        // revisions of the form '<tree>:<revision>', where <tree> is not
+        // necessarily |tree|. Get the latest revision, and favor those related
+        // to |tree|.
+        var _getLatestRevisionForTree = function(revisions) {

[ojan, 2014/08/23 01:23:06]

This should have a FIXME to sort by timestamp of the last (or first?) commit.

[Mathieu, 2014/08/24 23:42:11]

Done.

+          var i = 0;
+          var rev = revisions[revisions.length - 1 - i];
+          while (rev.lastIndexOf(tree, 0) !== 0) {
+            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) {
+          // In case of tree-revision equality, try a fallback.
+          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;
       },
 
       update: function() {
@@ -73,11 +97,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(range_group) {
+              this._processFailuresForRangeGroup(range_group, 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 +121,7 @@ found in the LICENSE file.
         return 0;
       },
 
-      _processFailuresForGroup: function(group, failures, annotations) {
-        var failuresByReason = {};
-
+      _processFailuresForRangeGroup: function(range_group, alerts, annotations) {

[ojan, 2014/08/23 01:23:05]

s/range_group/rangeGroup/

[Mathieu, 2014/08/24 23:42:11]

Done.

         var masterToTree = {};
         Object.keys(this._trees, function(tree, masters) {
           masters.forEach(function(master) {
@@ -105,8 +129,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 range_group 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 = {};
+        range_group.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,7 +152,12 @@ found in the LICENSE file.
             reason: reason,
           });
 
+          // Get the actual tree on which this failure occurred ('blink', 'chromium')

[ojan, 2014/08/23 01:23:06]

IMO, this is pretty clear. No need to give comments describing what the code
clearly does. Comments should be to explain high-level design or to explain why
the code is doing something non-obvious.

[Mathieu, 2014/08/24 23:42:11]

Sorry, I was annotating as I went along.

           var tree = masterToTree[failure.master_url];
+          if (!tree) {
+            // FIXME: No tree corresponding to master URL!

[ojan, 2014/08/23 01:23:05]

FIXMEs should state what to fix. Something like...
// FIXME: Figure out what tree masters that aren't in masterToTree should have.

[Mathieu, 2014/08/24 23:42:11]

Done.

+            return;

[ojan, 2014/08/23 01:23:05]

We don't actually want to early return here. We'll soon have the default mode in
sheriff-o-matic be to show all failures, even ones that don't have a tree. Was
not early returning causing JS errors or something?

[Mathieu, 2014/08/24 23:42:11]

Nah, it just didn't seem clean to be using |tree| as undefined here. 

[ojan, 2014/08/25 03:54:13]

Yeah. Long-term, the right thing here is to grab the tree name out of the JSON
that we grab off the server and to have the server ensure that there's always a
valid tree name. The problem with that right now is that a) the server uses the
wrong tree names and b) doesn't ensure a valid tree name in the same cases we
get undefined here. Both of those are pretty easy to fix.

Anyways, I filed crbug.com/406969 for this.

+          }
 
           // FIXME: Use a model class instead of a dumb object.
           if (!failuresByReason[failureKey])
@@ -140,13 +174,19 @@ found in the LICENSE file.
           };
         }.bind(this));
 
+        // Skip further processing if no failures were collected (e.g. the tree

[ojan, 2014/08/23 01:23:05]

Ditto: This is clearly what the next line of code does.

[Mathieu, 2014/08/24 23:42:11]

Done.

+        // was unknown).
+        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 +196,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 = range_group.sort_key;
+          this.failures[tree].push(
+              new CTFailureGroup(key, failures, range_group.likely_revisions,
+                                 annotations[key]));
         }.bind(this));
       },
     });