620046 - TestRun view should be combined with the HeatMap graph

Status: RESOLVED FIXED

(Tree Management Graveyard :: OrangeFactor, defect, P1)

Description

[Jeff Hammel]

The TestRun view is pretty much the same as the heatmap graph except
for the Y-axis, so combine the TestRun view with the HeatMap view
allowing y-axis choice (orange factor vs bug count) and having filter
drop downs.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

we should keep the code in place, but not display a heatmap.  We need to revisit the whole coloring scheme.

Comment 2

[Mark Côté [:mcote]]

I'm going to merge TestRun into OrangeFactor, keeping the OF-style graph but adding in the dropdowns and bug table from TestRun.

As a later patch, if I have time, rather than a Y-axis selector, I'll just do a multiple-line graph like the Bug view.

Comment 3

[Mark Côté [:mcote]]

Attachment: Combine TestRun and OrangeFactor views

Okay this is a big one.  Maybe the easiest way to review it is to look at the code with the patch applied--but I'll leave it up to you.

I combined the OF and Test Run views into the former; there's now no Test Run view.  Instead, the OF view has all the drop downs from the Test Run (defaulting to "All" for all categories) and the bug table, but has a line graph instead of the bar graph.  I also made the OF view have the same lines as Bug Details--bug count, bug frequency, and moving average.

I was able to factor our a lot of code from parseBugs() and put it into calculateMetric(), so all the stuff that calculates daily metrics is in one place now.

I moved the Bug Count view into woo.bugs.js, since there's no more sharing of the old displayHeatMap() function.  woo.heatmap.js now just contains all the old deprecated heat-map code but no display functions.

There's also a little bit of UI cleanup--I spaced out the OF dropdowns a little, and I removed the grey form background, which I think looks cleaner.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512169 [details] [diff] [review]
Combine TestRun and OrangeFactor views

r+ because I tested this and it looks good, but I would like some general answers to my concerns noted below.

Notes:
 - one hairy patch that rearranges a lot, so end result of testing is the best
 - I found that in updateMovAvg function we are doing a for loop each time this is called (for every data point we enter).  Can we be smarter about this?
 - the calculateMetric function is doing a lot and returning a lot.  Do we need all this information for each request?  It seems that we can either split this into two functions, or maybe pass a flag in to indicate what pieces we would need.
 - it seems that parseBugs we call calculateMetric for each bug!  I already think calculateMetric is too heavyweight, and calling it in a loop scares me.  
 - before calling calculateMetric we define this big list of variables to pass in and store results from the return.  It just seems like a lot of variables.

Despite my notes mentioned above, this works and feels as responsive as it was before, yet with more functionality!

Comment 5

[Mark Côté [:mcote]]

Attachment: A little more cleanup in calculateMetric() and parseBugs()

Good points.  I should first say that parseBugs() has *always* been doing all that calculateMetric() is now doing, including the moving-average loop and the the metric calculations.  However you are right that it could be at least somewhat improved.

> - I found that in updateMovAvg function we are doing a for loop each time this is called (for every data point we enter).  Can we be smarter about this?

Definitely.  I fixed that function to maintain a sum of the moving-average window.  No more looping.

> - the calculateMetric function is doing a lot and returning a lot.  Do we need all this information for each request?  It seems that we can either split this into two functions, or maybe pass a flag in to indicate what pieces we would need.

This is true.  We do need all that information for Orange Factor and for Bug Details, which construct 3-line charts.  We only need dateCounts for Top Bugs, and we only need the overall stats for the rest.  However, despite the fact that this function looks like it does a lot, it's actually very fast--like < 1 ms fast, on my machine at least.  Now that the loop in updateMovAvg() is gone, the only extra operations that are being performed is a couple quick arithmetic operations per date.  I don't think it's worth complicating this function by adding in various 'if' clauses  unless we really think it's slow--Premature Optimization is the Root of All Evil, etc.  Plus the bottleneck are the JSON calls here.

> - it seems that parseBugs we call calculateMetric for each bug!  I already think calculateMetric is too heavyweight, and calling it in a loop scares me.  

That is true, as it was before I refactored.  Only Top Bugs calls parseBugs() with multiple bugs.  True, it only needs dateCount and not the other two arrays, but again, I think these operations are pretty cheap, and having this stuff all in one place and not complicated by 'if' clauses is worth the extremely small penalty we're paying.

> - before calling calculateMetric we define this big list of variables to pass in and store results from the return.  It just seems like a lot of variables.

Sure is! :)  But since I wanted to refactor and not duplicate code, we have to pass in dataStartDate and dataEndDate and then remember the returned value of these, so that we have overall values at the end of parseBugs().  However there were a few variables that I accidentally left in after refactoring, so I took those out.

I also did a bit of renaming, for clarity.  Finally, I figure we always want to include zeros, not just in the Bug Details, on the off-hand chance that we have zero orange some day (even if it's just due to buildbot going down or something :).

So long story short--yes, calculateMetric() is sometimes doing more than it needs to, but I personally think that it's worth doing a few extra operations than to duplicate code or to make this function even more complicated by introducing more code paths.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512309 [details] [diff] [review]
A little more cleanup in calculateMetric() and parseBugs()

good clean fun here.  All looks good.

Comment 7

[Mark Côté [:mcote]]

Landed as http://hg.mozilla.org/automation/orangefactor/rev/cfab0b00f026

Comment 8

[Mark Côté [:mcote]]

I'll include a "Orange Factor for Selected View" subtitle in here too.

Comment 9

[Mark Côté [:mcote]]

Attachment: Include Orange Factor for selected view

This adds "Orange Factor for the Selected View" as a subtitle on the Orange Factor page.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 512520 [details] [diff] [review]
Include Orange Factor for selected view

nice and simple.  I wonder if this should be on other pages as well?

Comment 11

[Mark Côté [:mcote]]

Hmmm it might make sense on Bug Details, if people wanted to know the overall OF of that particular bug... and maybe Top Bugs.  But these are probably best put into other bugs, and probably not even P1.  Let's ship this thing! :)

Pushed as http://hg.mozilla.org/automation/orangefactor/rev/89a6d96da847