[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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237532|1                           |0
        is obsolete|                            |




--- Comment #10 from Alexey Proskuryakov <ap at webkit.org>  2014-09-02 17:06:02 PST ---
(From update of attachment 237532)
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

> 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)

?

> 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 :)

> 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.

> 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

> 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.

This style is not defined in CSS.

> 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?

> 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?

> 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.

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