Summary: | Display failing JSC stress tests in buildbot dashboard | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Srinivasan Vijayaraghavan <webkit> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, dburkart, jmarcell | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Bug Depends on: | 156920 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Srinivasan Vijayaraghavan
2016-04-14 13:48:55 PDT
Created attachment 276428 [details]
Patch
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.
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. 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? (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. 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. > 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. Created attachment 277955 [details]
Patch
Just realized that I still have some changes I want to make. Please disregard attachment 277955 [details].
Created attachment 278185 [details]
Patch for landing
Created attachment 278190 [details]
Patch
Created attachment 278628 [details]
Patch
Added unit tests! 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.
Created attachment 279403 [details]
Patch
Created attachment 279694 [details]
Patch
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 ")" Created attachment 279723 [details]
Patch
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. (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. Created attachment 279990 [details]
Patch for landing
Comment on attachment 279990 [details] Patch for landing Clearing flags on attachment: 279990 Committed r201475: <http://trac.webkit.org/changeset/201475> All reviewed patches have been landed. Closing bug. 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. Bug 170158: Fix dashboard test results on the dashboard https://bugs.webkit.org/show_bug.cgi?id=170158 |