[Webkit-unassigned] [Bug 136386] Update webkit dashboard to support performance bots

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136386





--- Comment #11 from Dana Burkart <dburkart at apple.com>  2014-09-02 18:09:16 PST ---
(In reply to comment #10)
> (From update of attachment 237532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237532&action=review
> 
> > Tools/ChangeLog:15
> > +        Support new 'performance' and 'perfType' keys.
> 
> Looks like the ChangeLog is stale.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:238
> > +        function collectPerfTestResults(data, stepName) {
> 
> s/Perf/Performance/g
> 

OK

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:253
> > +            if (!testStep.results || !testStep.results[0]) {
> 
> Should this be
> 
> if (!testStep.results || testStep.results[0] === BuildbotIteration.SUCCESS)
> 
> ?
> 

No, we're actually testing for failure of any kind, not success (failure is non-zero).

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:263
> > +            if (!testResults.failureCount) {
> > +                testResults.errorOccurred = true;
> > +            }
> 
> We don't use braces around single line blocks.
> 
> This code looks super confusing at first (why? no failures means that an error occurred?). Can you explain this situation in a comment? If you do, it won't be single line any more, so you should keep the braces :)
> 

OK. I was following the style of some similar code. The reason is that we should have already either hit a success *or* failure case, and if we haven't something is wrong, so we set 'errorOccurred' instead of failure

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:318
> > +        var webkitPerformanceTestResults = collectTestResults.call(this, data, "perf-test");
> 
> collectPerfTestResults or collectTestResults? If it's the latter, it needs a comment explaining why, or someone will "fix" it for consistency.
> 

Good point. It's correct, although strange, so I'll add a comment. collectPerfTestResults() is for compatibility with the internal buildbots.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:28
> > +    BuildbotQueueView.call(this, [], queues);
> 
> We should probable relieve BuildbotQueueView of knowing the distinction between debug and release queues (not in this patch).
> 
> All subclasses add their own logic on top of it, sometimes fighting with BuildbotQueueView for control.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:44
> > +        function appendPerfQueueStatus(queue)
> 
> s/Perf/Performance/g
> 

OK

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:91
> > +                releaseLabel.textContent = queue.perfType ? queue.perfType : label;
> 
> label should probably be named "defaultLabel" or something. Not seeing how perfType is used, I'm not sure about the intended design.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:119
> > +        this.addLinkToRow(row, "dasboard-link", "Performance Dashboard", queue.buildbot.perf_dashboard_url);
> 
> Typo: dasboard.
> 

Good catch.

> This style is not defined in CSS.
> 

Nor are many of the other arbitrary classes added to the procedurally generated HTML, I am simply following the same style.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueue.js:44
> > +    this.perfType = info.perfType || null;
> 
> s/perf/performance/g
> 
> But perfType doesn't appear to be set anywhere. Is this just dead code, or did you intend to set it?
> 

It is not set on the OpenSource dashboard, as we have only one type of performance bot on build.webkit. It is used for the internal configuration to change the heading (which normally says 'Release')

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:54
> > +        "GTK Linux 64-bit Release (Perf)": {platform: Dashboard.Platform.LinuxGTK, debug: false, tester: true, testCategory: Buildbot.TestCategory.Performance},
> > +        "EFL Linux 64-bit Release WK2": {platform: Dashboard.Platform.LinuxEFL, tester: true, testCategory: Buildbot.TestCategory.WebKit2},
> > +        "EFL Linux 64-bit Release WK2 (Perf)": {platform: Dashboard.Platform.LinuxEFL, performance: true}
> 
> I couldn't understand the difference between a performance bot like the GTK one, and a testCategory: Buildbot.TestCategory.Performance bot like the EFL one.
> 
> How does one go about figuring this out?
> 

That's actually a mistake on my part, good catch! They should both look like the EFL entry.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:65
> > +    perf_dashboard_url: "https://perf.webkit.org",
> 
> Coding style nit: should be performanceDashboardURL, not perf_dashboard_url.
> 
> It would be nice to make this a getter function for consistency with other URL getters.

OK.

Thanks for then review Alexey, I'll have a new patch addressing these concerns up shortly.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.