[webkit-reviews] review granted: [Bug 174777] Update Bot Watcher's Dashboard for buildbot 0.9 : [Attachment 316686] Updated patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 31 16:35:31 PDT 2017


Daniel Bates <dbates at webkit.org> has granted Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 174777: Update Bot Watcher's Dashboard for buildbot 0.9
https://bugs.webkit.org/show_bug.cgi?id=174777

Attachment 316686: Updated patch

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




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

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

This patch looks good. We could take this opportunity to modernize the code we
are modifying using ES6 enchantments. This is not strictly necessary to do in
this patch. We may want to consider instantiating and retuning new objects from
_adjustBuildDataIfNeeded() instead of mutating the existing one as mutating an
existing object can be error prone.

> Tools/ChangeLog:3
> +	   Update Bot Watcher's Dashboard for buildbot 0.9

buildbot => Buildbot

(Please fix all such occurrences. There are many occurrences throughout this
ChangeLog entry.)

> Tools/ChangeLog:10
> +	   (Buildbot.prototype._computeBuilderNameToIDMap): Fetch the builder
name to ID mapping from buidbot and store

buidbot => Buildbot

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbot.js:172
> +	   // FIXME: Remove buildbotNineIDMap lookup after
<https://github.com/buildbot/buildbot/issues/3465> is fixed.

buildbotNineIDMap => this._builderNameToIDMap

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:100
> -    return property[0] === "got_revision" && typeof property[1] ===
"object";
> +    return typeof property[0] === "object";

This makes the code a bit more more error prone so long as we still support
Buildbot less than version 0.9. For your consideration, I suggest that we have
this function take whether we are using Buildbot 0.9 or less and do what we do
now if it is less than 0.9.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:113
> -    //
["got_revision",{"Internal":"1357","WebKitOpenSource":"2468"},"Source"]
> +    // [{"Internal":"1357","WebKitOpenSource":"2468"},"Source"]
>      // OR
> -    // ["got_revision","2468","Source"]
> +    // ["2468","Source"]

We haven't made the switch to Buildbot 0.9 yet. So, we should not change this
comment to reflect the Buildbot 0.9 format until then. For now, I suggest we
update this comment to document both formats.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:114
>      if (isMultiCodebaseGotRevisionProperty(property))

I would pass this.queue.buildbot.VERSION_LESS_THAN_09 to this function. See my
remark in isMultiCodebaseGotRevisionProperty() for more details.

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

We may want to add a console.assert() above this block to ensure either
data.sourceStamp or data.sourceStamps is defined when using Buildbot version
less than 0.9.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:276
> +	       /* Sample masterShellCommandStep.urls data:
> +		* "urls": [
> +		*     {
> +		*	  "name": "view results",
> +		*	  "url": "/results/Apple Sierra Release WK2
(Tests)/r220013 (3245)/results.html"
> +		*     }
> +		* ]
> +		*/

Please use C++-style comments (starts with //).

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:312
> +    _adjustBuildDataIfNeeded: function(data)

Maybe a better name for this function would be _adjustBuildDataForBuildbot09.
For your consideration I suggest that we add a FIXME comment above this
function to remove it once we switch to Buildbot 0.9 or later.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:318
> +	   data.started_at = data.times[0];
> +	   data.complete_at = data.times[1];

We should delete property times to prevent a programming mistake.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:322
> +	   var revisionProperty = data.properties.findFirst(function(property)
{
> +	       return property[0] === "got_revision";
> +	   });

This is OK as-is. We could modernize this by using let and a ES6 arrow
function:

let revisionProperty = data.properties.findFirst((property) => property[0] ===
"got_revision");

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:328
> +	       revisionProperty.splice(0, 1);

We should console.assert(revisionProperty[0] === "got_revision").

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:332
> +	   for (var i = 0; i < data.steps.length; i++) {

We can simplify this code and remove the need for an index by using an ES6
for-of loop:

for (let step of data.steps) {
    ...
}

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:333
> +	       data.steps[i].complete = data.steps[i].isFinished;

We should delete isFinished from the step to prevent a programming mistake.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:336
> +	       data.steps[i].results = data.steps[i].results[0];  // See URL
http://docs.buildbot.net/latest/developer/results.html

Nit: There should only be one spec between the ';' and the start of the
comment.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:339
> +	   var masterShellCommandStep = data.steps.findFirst(function(step) {
return step.name === "MasterShellCommand"; });

This is OK as-is. We could write this using let and an ES6 arrow function: 

let masterShellCommandStep = data.steps.findFirst((step) => step.name ===
"MasterShellCommand");

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:343
> +

Minor: I do not see the need for this empty line. One empty line is sufficient
to demarcate the block above from the block below.

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:344
> +	   data.state_string = data.text.join(" ");

We should delete the property text to prevent a programming mistake,

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotIteration.js:437
> +	       /* Sample data for a single build:
> +		* "builds": [
> +		*     {
> +		*	  "builderid": 282,
> +		*	  "buildid": 5609,
> +		*	  "complete": true,
> +		*	  ...
> +		*	  "workerid": 188
> +		*     }
> +		* ]
> +		*/

Please use C++ style comments (that start with //).

>
Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/B
uildbotQueue.js:245
> +	   return (data.builds[index].number);

The parentheses are not needed.


More information about the webkit-reviews mailing list