Bug 175593

Summary: undefined URL in PopoverTracker for failed step
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, dbates, lforschler, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch
none
Updated patch
dbates: review+
Patch for landing none

Description Aakash Jain 2017-08-15 12:48:10 PDT
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>.
Comment 1 Aakash Jain 2017-08-15 12:53:29 PDT
Created attachment 318153 [details]
Proposed patch
Comment 2 Aakash Jain 2017-08-15 13:57:44 PDT
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.
Comment 3 Daniel Bates 2017-08-15 14:06:49 PDT
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.
Comment 4 Daniel Bates 2017-08-15 14:08:08 PDT
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.
Comment 5 Daniel Bates 2017-08-15 14:08:42 PDT
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.
Comment 6 Aakash Jain 2017-08-15 15:30:58 PDT
Created attachment 318183 [details]
Patch for landing

Incorporated review comments.
Comment 7 WebKit Commit Bot 2017-08-15 15:58:21 PDT
Comment on attachment 318183 [details]
Patch for landing

Clearing flags on attachment: 318183

Committed r220771: <http://trac.webkit.org/changeset/220771>
Comment 8 WebKit Commit Bot 2017-08-15 15:58:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-08-15 15:59:13 PDT
<rdar://problem/33906587>