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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 2 19:20:36 PDT 2014


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
 Attachment #237540|review?                     |review+, commit-queue-
               Flag|                            |

--- Comment #14 from Alexey Proskuryakov <ap at webkit.org>  2014-09-02 19:20:39 PST ---
(From update of attachment 237540)
View in context: https://bugs.webkit.org/attachment.cgi?id=237540&action=review

> I can change it in both places if you want.

Yes, I think that comparing to BuildbotIteration.SUCCESS reads better. The existing place probably pre-dates adding these named constants.

> Tools/ChangeLog:18
> +        Support new 'performance' and 'perfType' keys.

Stale ChangeLog, perfType is no more.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:259
> +            testResults.failureCount = (testStep.results[0]) ? 1 : 0;

I think that this is not quite how the other result objects work. Failure count is for steps that actually have failure counts (like layout tests).

Besides, what are the possible values for testStep.results[0] here? We know that it's not 0, because that case is handled above. How else can testStep.results[0] evaluate to false? This needs a comment if that can actually happen.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:315
> +        // This is actually correct, we're calling collectTestResults vs. collectPerformanceTestResults because of
> +        // differences in the internal buildbot.

"This is actually correct" is superfluous, you can just say "We are calling collectTestResults and not collectPerformanceTestResults because of differences in the internal buildbot."

A little more detail about what the differences are could be appropriate (cf. how we explain revision number formats for internal bots with two source trees).

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:91
> +                releaseLabel.textContent = queue.performanceType ? queue.performanceType : label;

This still seems a little confusing. What is "performance type", that doesn't parse.

Perhaps performanceTestName would be more descriptive?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotPerformanceQueueView.js:116
> +        content.className = "ews-popover";

Didn't notice this at first - since it's not a EWS popover, please don't use its class names. Duplicating style rules is OK.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js:66
> +    performanceDashboardURL: function()

I now think that I gave the wrong advice here. I was thinking of a getter function, but we have regular functions when there is an argument, and simple attributes like baseURL when there is none.

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

More information about the webkit-unassigned mailing list