Bug 142211 – build.webkit.org/dashboard: Don't repeatedly handle each test type

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

We hardcode all test types in several places in the dashboard code, but we don't do anything unique for any of them, except for layout tests. It would be much better to handle all these uniformly.

[Alexey Proskuryakov]

Created attachment 247749 [details]
proposed patch

See also: rdar://problem/19803991.

[Matthew Hanson]

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?

[Alexey Proskuryakov]

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.

[Tim Horton]

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

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r180959>.