Bug 156595 – Display failing JSC stress tests in buildbot dashboard

• Component: New Bugs
• Status: RESOLVED FIXED

[Srinivasan Vijayaraghavan]

Display failing JSC stress tests in buildbot dashboard

[Srinivasan Vijayaraghavan]

Created attachment 276428 [details]
Patch

[WebKit Commit Bot]

Attachment 276428 [details] did not pass style-queue:


ERROR: Tools/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:19:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:26:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:28:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:29:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:33:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Tools/ChangeLog:34:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 13 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 276428 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:304
> +        // Work around bug 80159: -webkit-user-select:none not respected when copying content.
> +        // We set clipboard data manually, temporarily making non-selectable content hidden
> +        // to easily get accurate selection text.

This is copied code, please factor it out.

[Alexey Proskuryakov]

Comment on attachment 276428 [details]
Patch

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

Despite my many comments, this looks very good, almost ready for landing.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:49
> +    this.javaScriptCoreStressTestResults = null;

Is "stress test" the correct name for this test suite? We simply execute run-javascriptcore-tests, there doesn't seem to be anything about "stress" there.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:193
> +        this.regressions = data.jscStressFailList;
> +        console.assert(data.numJSCStressFailures === this.regressions.length);

I think that before having this go live, we should take another look at JSON content, to make it better match WebKit naming conventions.

Would something like this make sense?

numJSCStressFailures -> failureCount
jscStressFailList -> failures

But also, why do we need a separate count when there it can be taken from the array?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:108
> +                    } else if (failedStep.name === "jscore-test") {

As a separate patch, we should also teach the dashboard how to display failures in the popover when there are failures on multiple kinds.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:109
> +                        var status = new StatusLineView(messageElement, StatusLineView.Status.Bad, this._testStepFailureDescription(failedStep), failedStep.tooManyFailures ? failedStep.failureCount + "\uff0b" : failedStep.failureCount, iteration.queue.buildbot.javaScriptCoreStressTestFailuresURLForIteration(iteration));

I don't think that JSC tests have the concept of "too many failures" at this time

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:268
> +        this._addIterationHeadingToPopover(iteration, content, "jscore stress test failures");

Once again, unsure about the name.

I don't see any link from the popover to detailed test results, is there one? It's useful to have an easy way to see the actual output.

We'll probably need a different UI than for layout tests, because we don't have individual result files here.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:360
> +        if (iteration.javaScriptCoreStressTestResults)
> +            var content = this._popoverContentForJavaScriptCoreStressTestRegressions(iteration);
> +        else {

Is there anything here that could be shared rather than copy/pasted?

[Srinivasan Vijayaraghavan]

(In reply to comment #4)
> Comment on attachment 276428 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276428&action=review
> 
> Despite my many comments, this looks very good, almost ready for landing.
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:49
> > +    this.javaScriptCoreStressTestResults = null;
> 
> Is "stress test" the correct name for this test suite? We simply execute
> run-javascriptcore-tests, there doesn't seem to be anything about "stress"
> there.
Only stress test info is sent to BuildBot right now. 

I can modify the run-javascriptcore-tests script to include API test failures and build failures as well.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:193
> > +        this.regressions = data.jscStressFailList;
> > +        console.assert(data.numJSCStressFailures === this.regressions.length);
> 
> I think that before having this go live, we should take another look at JSON
> content, to make it better match WebKit naming conventions.
> 
> Would something like this make sense?
> 
> numJSCStressFailures -> failureCount
> jscStressFailList -> failures
> 
> But also, why do we need a separate count when there it can be taken from
> the array?
Count is redundant, I'll get rid of it. The JSON might further change once I add the non-stress JSC results as well.

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:108
> > +                    } else if (failedStep.name === "jscore-test") {
> 
> As a separate patch, we should also teach the dashboard how to display
> failures in the popover when there are failures on multiple kinds.
OK

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:109
> > +                        var status = new StatusLineView(messageElement, StatusLineView.Status.Bad, this._testStepFailureDescription(failedStep), failedStep.tooManyFailures ? failedStep.failureCount + "\uff0b" : failedStep.failureCount, iteration.queue.buildbot.javaScriptCoreStressTestFailuresURLForIteration(iteration));
> 
> I don't think that JSC tests have the concept of "too many failures" at this
> time
Will be removed, thanks!

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:268
> > +        this._addIterationHeadingToPopover(iteration, content, "jscore stress test failures");
> 
> Once again, unsure about the name.
> 
> I don't see any link from the popover to detailed test results, is there
> one? It's useful to have an easy way to see the actual output.
> 
> We'll probably need a different UI than for layout tests, because we don't
> have individual result files here.
I'll change the text.

There's no link to the JSC test stdio (the heading does include a link to the build page though). It would be trivial to add, but we need to figure out a good place in the UI for it. Perhaps to the right of the build link?

> > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:360
> > +        if (iteration.javaScriptCoreStressTestResults)
> > +            var content = this._popoverContentForJavaScriptCoreStressTestRegressions(iteration);
> > +        else {
> 
> Is there anything here that could be shared rather than copy/pasted?
Do you mean a reusable function that displays a loading indicator until content is loaded?

I would like to make server-side changes (i.e. run-javascriptcore-tests JSON content changes, including non-stress failures, plus the buildbot change to make it live) first. I've been resorting to weird hackery to check if the front-end changes work, but it will be much faster if the back-end works.

[Srinivasan Vijayaraghavan]

To clarify that last bit:
I'll submit a patch for the back-end first. Having the new endpoint will allow for faster iteration of the dashboard front-end.

[Alexey Proskuryakov]

> Only stress test info is sent to BuildBot right now. 
>
> I can modify the run-javascriptcore-tests script to include API test failures and build failures as well.

It seems worth having the code slightly more generic. We used to have other non-stress tests, we have the JS API tests, and we could get more. OR maybe we should run these as separate buildbot steps.

> > Is there anything here that could be shared rather than copy/pasted?
> Do you mean a reusable function that displays a loading indicator until content is loaded?

That seems like a good one to have shared, yes.

[Srinivasan Vijayaraghavan]

Created attachment 277955 [details]
Patch

[Srinivasan Vijayaraghavan]

Just realized that I still have some changes I want to make. Please disregard attachment 277955 [details].

[Srinivasan Vijayaraghavan]

Created attachment 278185 [details]
Patch for landing

[Srinivasan Vijayaraghavan]

Created attachment 278190 [details]
Patch

[Srinivasan Vijayaraghavan]

Created attachment 278628 [details]
Patch

[Srinivasan Vijayaraghavan]

Added unit tests!

[Srinivasan Vijayaraghavan]

Comment on attachment 278628 [details]
Patch

Realized that this will basically not work. I'll fix and re-upload, so no need to review this one.

[Srinivasan Vijayaraghavan]

Created attachment 279403 [details]
Patch

[Srinivasan Vijayaraghavan]

Created attachment 279694 [details]
Patch

[Dean Johnson]

Comment on attachment 279694 [details]
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 ")"

[Srinivasan Vijayaraghavan]

Created attachment 279723 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 279723 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:114
> +                        var URL = mostRecentFinishedIteration.queue.buildbot.javaScriptCoreTestStdioURLForIteration(mostRecentFinishedIteration, failedStep.name);

WebKit style is to have local variable names start with lower case, so this one should be "url", not "URL".

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

I wouldn't have added parentheses around additionalTextTarget, as it is already so easy to figure out wha the condition is.

We don't have a specific style rule about this, but we have that in an example in the style guide ("return condition ? 1 : 0;")

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:108
> +                    } else if (failedStep.name in ["jscore-test", "webkit-32bit-jsc-test", "webkit-jsc-cloop-test"]) {

In a separate patch, we should also present a better popover when there are both layout test and jsc failures.

[Srinivasan Vijayaraghavan]

(In reply to comment #19)
> In a separate patch, we should also present a better popover when there are
> both layout test and jsc failures.
Created Bug 158163 to track this.

[Srinivasan Vijayaraghavan]

Created attachment 279990 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 279990 [details]
Patch for landing

Clearing flags on attachment: 279990

Committed r201475: <http://trac.webkit.org/changeset/201475>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jason Marcell]

I believe this broke the dashboard unit tests.

You can run the tests with
open Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/tests/index.html

or run-dashboard-tests

I'd like to work on having EWS run run-dashboard-tests, but for now it does not.

[Jason Marcell]

Bug 170158: Fix dashboard test results on the dashboard
https://bugs.webkit.org/show_bug.cgi?id=170158