[Webkit-unassigned] [Bug 156595] Display failing JSC stress tests in buildbot dashboard

[bugzilla-daemon at webkit.org]

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

--- Comment #17 from Dean Johnson <dean_johnson at apple.com> ---
Comment on attachment 279694
  --> https://bugs.webkit.org/attachment.cgi?id=279694
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279694&action=review

If I were you, I'd also change all instances of "javaScriptCore" to "jsc" and "JavaScriptCore" to "JSC". I think it's well enough known what JSC represents to assume the reader will know what's happening in the code, but it'd probably be wise to get opinions from others before taking that suggestion.

Overall, it looks pretty good!

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:114
> +                        var statusView = new StatusLineView(message, StatusLineView.Status.Bad, this._testStepFailureDescription(failedStep), failedStep.failureCount, mostRecentFinishedIteration.queue.buildbot.javaScriptCoreTestStdioURLForIteration(mostRecentFinishedIteration, failedStep.name));

This line is really long and somewhat difficult to read. Can we store `mosRecentFinishedIteration.queue.buildbot.javascriptCoreTestStdioURLForIteration(mostRecentFinishedIteration, failedStep.name)` into a variable and use that as the argument instead of in-lining?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:143
> +                    var statusView = new StatusLineView(message, StatusLineView.Status.Good, firstRecentUnsuccessfulIteration ? "last succeeded" : statusMessagePassed, null, url);

I realize that this existed before you edited it, but can we not inline the conditional and instead store it in a variable? It would significantly improve readability, in my opinion.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:147
> +                    this.element.appendChild(statusView.element);

Ditto above

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:363
> +        if (this.queue.buildbot.needsAuthentication && this.queue.buildbot.authenticationStatus === Buildbot.AuthenticationStatus.InvalidCredentials)

Shouldn't this be this.queue.buildbot.needsAuthentication || this.queue.buildbot.authenticationStatus === Buildbot.AuthenticationStatus.InvalidCredentials?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:257
> +            var elementType = (additionalTextTarget)? "a" : "span";

Nit: Space after ")"

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160524/6e84d2dc/attachment.html>