Bug 146210 – Allow other projects to display on the dashboard

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Jason Marcell]

Currently on projects with an opensource or internal SVN revision are able to be displayed on the bot watcher's dashboard. There are other projects that do not exist in either of those two repositories (iOS git projects, in particular) which should be allowed to have an entry on the bot-watcher's dashboard.

[Jason Marcell]

Created attachment 255728 [details]
Patch to allow for projects other than open source and internal on the dashboard.

[Daniel Bates]

Comment on attachment 255728 [details]
Patch to allow for projects other than open source and internal on the dashboard.

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:116
> +    if (isGitGotRevisonProperty(property, value))
> +        return value;
> +    else
> +        return parseInt(value);

We should have this function return a string and make it the caller's responsibility to parse the string for a number if needed. Then we can remove the function isGitGotRevisonProperty().

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:212
> +        for (project in this.queue.branch) {

This code is assuming that this.queue.branch represents a dictionary of (project, branch name)-pairs. Currently, this.queue.branch represents a dictionary of (repository, branch name)-pairs. Is this acceptable?

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

Notice that we compute this on each iteraiton. I suggest we move this out of the loop.

Also, the first disjunct is unnecessary by definition of isMultiCodebaseGotRevisionProperty().

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:222
> +                this.otherRevision = parseRevisionProperty(revisionProperty, project, "Tools");

Is it correct to fallback to using key Tools?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:211
> +        if (iteration.otherRevision)
> +            content.textContent = iteration.otherRevision.substring(0, 8);

This code assumes that iteration.otherRevision is always a Git revision. Is this correct?

How did you choose 8?

Did you mean to use String.substring() which takes a from and to argument for the indices?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:267
> +            return this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, true);

How does this work if previousDisplayedIteration is non-null?

[Jason Marcell]

Created attachment 256746 [details]
Patch to allow for projects other than open source and internal on the dashboard.

Dan, I think I've addressed most of your concerns.

One that I'm not sure how to address is this question:
This code is assuming that this.queue.branch represents a dictionary of (project, branch name)-pairs. Currently, this.queue.branch represents a dictionary of (repository, branch name)-pairs. Is this acceptable?

I'm not sure whether this is an issue, but it might be worth discussing.

[Daniel Bates]

Comment on attachment 256746 [details]
Patch to allow for projects other than open source and internal on the dashboard.

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotCombinedQueueView.js:74
> -            var status = new StatusLineView(message, StatusLineView.Status.Good, "all tests passed", null, null);
> +            var statusMessagePassed = "all " + (queue.builder ? "builds" : "tests") + " passed"
> +            var status = new StatusLineView(message, StatusLineView.Status.Good, statusMessagePassed, null, null);

I'm unclear how this change relates to the purpose of this bug. Although this change is straightforward, we may want to consider making this change in a separate bug/patch.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:202
>          // revision. Therefore, we only look at got_revision to extract the Internal revision when it's
>          // a dictionary.

The last paragraph of comment block above is out-of-date with the proposed code change. Either we should update the comment or remove the last paragraph from the comment block above.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:213
> +        for (project in this.queue.branch) {
> +            if (project === "openSource")
> +                this.openSourceRevision = parseInt(parseRevisionProperty(revisionProperty, "WebKit", "opensource"));
> +            else if (project === "internal")
> +                this.internalRevision = parseInt(parseRevisionProperty(revisionProperty, "Internal", "internal"));
> +            else
> +                this.otherRevision = parseRevisionProperty(revisionProperty, project, "Internal");

This does not seem correct. Assume this.queue.branch := {openSource: "trunk, internal: "trunk"} (which it is for all WebKit OpenSource Project queues by (*) and (**). Suppose revisionProperty = "2468". Then this.openSourceRevision, this.internalRevision = 2468. How does the dashboard behave?

(*) Tthere is no WebKit OpenSource Project queues that explicitly specify the property branch in <http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/WebKitBuildbot.js?rev=184879>.
(**) <http://trac.webkit.org/browser/trunk/Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/Buildbot.js#L105>

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:212
> +            // Using substr for Git Short-SHA

Either elaborate further on the non-obviousness of using String.substr() to truncate a SHA or remove this coment.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:215
> +                content.textContent = content.textContent.substr(0, 8);

How did you choose 8? I mean, did you compute the collision probability as part of coming up with 8. Throughout <http://git-scm.com/book/ca/v1/Git-Tools-Revision-Selection#Short-SHA> and in various Git literature and tool output I see SHAs of length 7.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotQueueView.js:271
> +            return this._revisionContentWithPopoverForIteration(iteration, previousDisplayedIteration, true);

It is not clear here what the purpose of the boolean true is in the last argument. I suggest we define a local variable, say isInternalRevision, defined to be true and pass this named local variable instead of using the keyword true directly in the call to BuildbotQueueView.prototype._revisionContentWithPopoverForIteration().

> Tools/ChangeLog:3
> +        https://bugs.webkit.org/show_bug.cgi?id=146210

Missing bug title, which should be above this line.