557552 - add ability to view performance test results between two pushes

Status: RESOLVED FIXED

(Tree Management Graveyard :: TBPL, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

finding the performance difference between two revisions is cumbersome business with TBPL. lots of clicking around, and copy/pasting testnames and result numbers just to compare a single test's results between two pushes.

this patch provides simple UI for viewing at-a-glance all results for all completed performance tests between two pushes.

usage:
- check two boxes to trigger the comparison
- click the resulting table to close it

i have a few improvements planned to make it easier to use (noted in the patch) but it's far enough along to be useful as it is.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: screenshot - checkboxes

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: screenshot - results table

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: v2

moved the URL abstraction out to bug 557641.

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 437403 [details] [diff] [review]
v2

First of all, thank you very much for doing this!

>       '<h2><span class="pusher">' + push.pusher + '</span> &ndash; ' +
>-      '<span class="date">' + self._getDisplayDate(push.date) + '</span></h2>\n' +
>+      '<span class="date">' + self._getDisplayDate(push.date) + '</span>' +
>+      ' (compare: <input class="revsToCompare" id="compareRevs" type="checkbox" value="' + push.patches[0].rev + '" onclick="compare()">)' +
>+      '</h2>\n' +

Please set up the click handler like the other ones (see for example _installMachineResultClickHandler).

>diff --git a/script.js b/script.js

Don't put the compare function into script.js. Move this stuff into UserInterface.js or create a new file, for example PerformanceComparator.js or something like that.

>+  for (var i in revsEls)

Use Array.forEach(revsEls, function ...). Only use for...in for objects. See the note on https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description for reference.

>+    if (revsEls[i].checked) revs.push(revsEls[i].getAttribute("value"));

Give the if body its own line and use .value instead of .getAttribute("value").

>+  var oss = Controller._data.getOss();
>+  var revResults = [];
>+  var pushes = Controller._data.getPushes();
>+  pushes.forEach(function(push) {
>+    var rev = push.patches[0].rev;
>+    if (revs.some(function(r) r == rev)) {

Data.js has _getPushForRev.

>+      var revResult = { rev: rev };
>+      oss.forEach(function(os) {
>+        revResult[os] = {};
>+        if (push.results && push.results[os]) {
>+          for (var buildType in push.results[os]) {
>+            if (buildType != "Talos") continue;
>+            var buildResults = push.results[os][buildType];
>+            buildResults.forEach(function(buildResult) {
>+              var testResults = Controller._data.getMachineResults()[buildResult.runID].getTestResults();
>+              if (testResults) {
>+                testResults.forEach(function(testResult) {
>+                  revResult[os][testResult.name] = testResult.result;
>+                });
>+              }
>+            });
>+          }
>+        }
>+      });
>+      revResults.push(revResult);
>+    }
>+  });

I think this should all be done somewhere in Data.js, probably after _combineResults, once for all pushes. You can set a perfResults property on every push object.

>+  var osNames = {"linux": "Linux", "linux64": "Linux64", "osx": "Mac OS X", "osx64": "Mac OS X64", "windows": "Windows"};

This needs to be factored out into Config.js.

>+  var testNames = [
>+    "tdhtml",
...
>+    "twinopen"
>+  ];

This list needs to live somewhere else, maybe in TinderboxData.js.

>+  div.setAttribute("style", "position: absolute; z-index: 1000; left: 0px; top: 0px; background-color: #ffffff; border: 1px solid black; font-size: 0.6em;");

Please put this into style.css and set an ID on the div.

>+    var revsEls = document.getElementsByClassName("revsToCompare");
>+    for (var i in revsEls) revsEls[i].checked = false;

I think with the help of jQuery this could become:
$(".revsToCompare").each(function { this.checked = false; })
... or maybe even shorter.

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)
> >+  for (var i in revsEls)
> 
> Use Array.forEach(revsEls, function ...). Only use for...in for objects. See
> the note on
> https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description
> for reference.

It's a NodeList, not an array.

Other changes seem fine, new patch coming.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v3

Comments addressed. All UI stuff is now separated out into UserInterface.js.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #5)
> (In reply to comment #4)
> > >+  for (var i in revsEls)
> > 
> > Use Array.forEach(revsEls, function ...). Only use for...in for objects. See
> > the note on
> > https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference/Statements/For...in#Description
> > for reference.
> 
> It's a NodeList, not an array.

That means you can't do revsEls.forEach(...), but you can still do Array.forEach(revsEls, ...) if I recall correctly.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: v3.1

Yep, works on NodeLists.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 439183 [details] [diff] [review]
v3.1

Thanks!

Comment 10

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/1d8387bacfbb

Comment 11

[Markus Stange [:mstange]]

Could you also check in PerformanceComparator.js, please? :-)
I should have noticed that in the review...

Backed out for now.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v3.2

with file attached

Comment 13

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/d214660065d3

Comment 14

[Jeff Walden [:Waldo]]

Does tbpl implement Array.forEach if it's not already implemented?  It's a non-standard extension (not entirely sure why it wasn't in ES5 to complement the prototype-based counterparts, to be honest), so you need to add it to keep tbpl working in webkit-based browsers, like so somewhere:

if (!Array.forEach)
{
  Array.forEach = function (arraylike, fun, thisp)
  {
    return Array.prototype.forEach.call(arraylike, fun, thisp);
  };
}

Comment 15

[Markus Stange [:mstange]]

It doesn't, so maybe we should just use some jQuery magic.