[webkit-reviews] review granted: [Bug 174872] Dashboard: Don't crash when expected elements are not present in buildbot data : [Attachment 316471] Proposed patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 174872: Dashboard: Don't crash when expected elements are not present in
buildbot data
https://bugs.webkit.org/show_bug.cgi?id=174872

Attachment 316471: Proposed patch

https://bugs.webkit.org/attachment.cgi?id=316471&action=review




--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 316471
  --> https://bugs.webkit.org/attachment.cgi?id=316471
Proposed patch

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

Can we write a unit test for this change?

> Tools/ChangeLog:3
> +	   Dashboard: Don't crash when expected elements are not present in
buildbot data

"Crash" is not the correct terminology to use to refer to a JavaScript
exception or error.

> Tools/ChangeLog:9
> +	   (BuildbotIteration.prototype.failureLogURL): check if
_firstFailedStep.logs is present before accessing

Please capitalize the first letter in this sentence.

> Tools/ChangeLog:11
> +	   (BuildbotIteration.prototype._parseData): check if data.sourceStamps
is present before accessing its length.

Ditto.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:247
> +	   else if (data.sourceStamps)

Is it possible for the version of Buildbot we use in OpenSource or internally
to not have both sourceStamp and sourceStamps defined. If not, then I suggest
we defer making this change until we upgrade OpenSource to the latest version.
I know that the latest Buildbot does not have this property from talking with
you today.