Closed Bug 557552 Opened 14 years ago Closed 14 years ago

add ability to view performance test results between two pushes

Categories

(Tree Management Graveyard :: TBPL, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dietrich, Assigned: dietrich)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
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.
Attached image screenshot - checkboxes
Attachment #437331 - Attachment is patch: true
Attachment #437331 - Attachment mime type: application/octet-stream → text/plain
Attached patch v2 (obsolete) — Splinter Review
moved the URL abstraction out to bug 557641.
Attachment #437331 - Attachment is obsolete: true
Depends on: 557641
Assignee: nobody → dietrich
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.
(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.
Attached patch v3 (obsolete) — Splinter Review
Comments addressed. All UI stuff is now separated out into UserInterface.js.
Attachment #437403 - Attachment is obsolete: true
(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.
Attached patch v3.1 (obsolete) — Splinter Review
Yep, works on NodeLists.
Attachment #439176 - Attachment is obsolete: true
Attachment #439183 - Flags: review?(mstange)
Comment on attachment 439183 [details] [diff] [review]
v3.1

Thanks!
Attachment #439183 - Flags: review?(mstange) → review+
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/1d8387bacfbb
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Could you also check in PerformanceComparator.js, please? :-)
I should have noticed that in the review...

Backed out for now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v3.2Splinter Review
with file attached
Attachment #439183 - Attachment is obsolete: true
Attachment #439332 - Flags: review+
http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/d214660065d3
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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);
  };
}
It doesn't, so maybe we should just use some jQuery magic.
Depends on: 559958
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.