undefined URL in PopoverTracker for failed step

[Aakash Jain]

When a test step fails (other than layout-test), PopoverTracker in the buildbot 0.9 dashboard links to undefined for that step. It should atleast link to the 

See <rdar://problem/33898769>.

[Aakash Jain]

Created attachment 318153 [details]
Proposed patch

[Aakash Jain]

Created attachment 318167 [details]
Updated patch

Created the URL for the exact step logs page.

Even thought the changes in BuildbotTesterQueueView.js are not mandatory for this to work, it is still good to have those changes to handle any unexpected scenario.

[Daniel Bates]

Comment on attachment 318167 [details]
Updated patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:316
> +    _urlForStep: function(step)

Maybe a better name for this would be stdioURLForStep as this function returns the URL to the stdio page for the step.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotIteration.js:326
> +        return this.queue.buildbot.buildPageURLForIteration(this) + "/steps/" + step.number + "/logs/stdio";

I suggest we add a FIXME comment above this line to explain that Buildbot 0.9 should provide this URL in the JSON output for the step as it does in Buildbot versions less than 0.9. Even better we should file a Buildbot bug (if one does not already exist) and reference it in this comment.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:115
> +                        var url = failedStep.URL ? failedStep.URL : iteration.queue.buildbot.buildPageURLForIteration(iteration);

I suggest we revert this change.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/dashboard/Scripts/BuildbotTesterQueueView.js:265
> +            else {
> +                var url = failedStep.URL ? failedStep.URL : iteration.queue.buildbot.buildPageURLForIteration(iteration);
> +                addResultKind(this._testStepFailureDescriptionWithCount(failedStep), url);
> +            }
>          }, this);

Ditto.

[Daniel Bates]

Comment on attachment 318167 [details]
Updated patch

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

> Tools/ChangeLog:7
> +

Please explain that this fix is a workaround for a Buildbot 0.9 bug where it does not provide a URL to stdio for a build step. Mention that Buildbot versions less than 0.9 always include a URL to the stdio of the step.

[Daniel Bates]

Comment on attachment 318167 [details]
Updated patch

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

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

Please add the radar bug URL under this line.

[Aakash Jain]

Created attachment 318183 [details]
Patch for landing

Incorporated review comments.

[WebKit Commit Bot]

Comment on attachment 318183 [details]
Patch for landing

Clearing flags on attachment: 318183

Committed r220771: <http://trac.webkit.org/changeset/220771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33906587>