| Summary: | build.webkit.org/dashboard: Don't repeatedly handle each test type | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||
| Component: | Tools / Tests | Assignee: | Alexey Proskuryakov <ap> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | dburkart, matthew_hanson, ossy, thorton | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Alexey Proskuryakov
2015-03-02 22:23:39 PST
Created attachment 247749 [details] proposed patch See also: rdar://problem/19803991. Comment on attachment 247749 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=247749&action=review Looks really good. Thank you for this refactoring! > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:32 > + this._parseResults(testStep); Why are this.name and this.URL not set in _parseResults? I'm not at all hung up on this point, just curious. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:91 > + It seems that you are passing undefined as the initial value of sum so that we can tell if we actually have a sum of 0 or if we had some sort of error condition. We could get around this by defining this.errorOccurred = False and setting the flag to True within resultsSummarizer if we encounter an error condition. I realize that this adds a side effect to resultSummarizer, but it would remove the need to include each of the properties set above in the check below. You could then also use 0 as the default value for sum in your calls to reduce. Once again, I'm not too picky about this implementation detail. > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:136 > + }, I know this was a mostly a copy/paste, but can we move the unicode strings to constants or add comments for readability? Comment on attachment 247749 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=247749&action=review >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:32 >> + this._parseResults(testStep); > > Why are this.name and this.URL not set in _parseResults? I'm not at all hung up on this point, just curious. The reason is historical, _parseResults is moved code. They should be there, I can move them before landing. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:91 >> + > > It seems that you are passing undefined as the initial value of sum so that we can tell if we actually have a sum of 0 or if we had some sort of error condition. We could get around this by defining this.errorOccurred = False and setting the flag to True within resultsSummarizer if we encounter an error condition. > > I realize that this adds a side effect to resultSummarizer, but it would remove the need to include each of the properties set above in the check below. You could then also use 0 as the default value for sum in your calls to reduce. > > Once again, I'm not too picky about this implementation detail. True. I'd like to keep it as is, as it's just moved code, and not a super big deal. >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:136 >> + }, > > I know this was a mostly a copy/paste, but can we move the unicode strings to constants or add comments for readability? Sure. Comment on attachment 247749 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=247749&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTestResults.js:143 > + // on retry (except for reftests), so many times, you will see images on buidbot page, but not on the dashboard. typo buidbot Committed <http://trac.webkit.org/r180959>. |