Bug 199810 – Provide build request status description information on dashboard.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Provide build request stauts detail information on dashboard.

Created attachment 374155 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 374155 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:10
> +        Update SQL for existing instance:
> +            UPDATE build_requests ADD COLUMN request_status_detail varchar(1024) DEFAULT NULL;

Now that we have other clients, can we create a new file like migration.sql where these instructions are conditionally executed?
We'd ideally create some kind of admin page like /admin/migrate.php to automatically run that.

> Websites/perf.webkit.org/init-database.sql:317
> +    request_status_detail varchar(1024) DEFAULT NULL,

Hm... I don't really like the name "status detail".
It doesn't say what kind of detail it has; that it's a human readable text.
How about status_description or status_label?

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:118
> +        const showWarningIcon = request.hasCompleted() && request.statusDetail();

Don't we want to show this when the request had failed as well?
We need to check hasFinished() instead for that.

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:126
> +        if (request.statusDetail() && !showWarningIcon)

This is bit convoluted condition.
Why don't we just say request.statusDetail() && !request.hasCompleted()?
That's way easier to reason about.
If anything, this condition should be named something like hasInProgressReport

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:127
> +            cellContent.push(element('label', ':'), element('label', `${request.statusDetail()}`));

This is not a correct use of label element. Just use a span.
Why ":" and the status detail need to be two different label elements? That doesn't seem right.

> Websites/perf.webkit.org/public/v3/components/warning-icon.js:13
> +        if (this._warningMessage)
> +            this.content('button').title = this._warningMessage;

Since #button is defined in ButtonBase, it would be better if ButtonBase had a method to set this instead.

Created attachment 374445 [details]
Patch

Created attachment 374446 [details]
Patch

Created attachment 374488 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 374488 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:8
> +        Add build request status detail to show detailed information for a build and show it in dashboard.

detail -> description?

> Websites/perf.webkit.org/init-database.sql:317
> +    request_status_description varchar(1024) DEFAULT NULL,

Let's not make the change to init-database.sql for now.
That is, just make the change to migrate-database.sql then make existing tools run both init-database.sql and migrate-database.sql
e.g. update initDatabase() in TestServer (server-tests/resources/test-server.js) to run migrate-database.sql in addition to init-database.sql.
Also, we should add an instruction to ReadMe.md about how we can migrate databases.
Eventually, we should version database schema but those two changes should be sufficient for now.

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:121
> +            cellContent.push(element('div', {class: 'warning-icon-container'}, new WarningIcon(`Last synced status description: ${request.statusDescription()}`)));

"Last synced status description:" is a very wordy label. How about just "Latest status: ${~}"?

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:242
> +            .description-cell span {

We don't want to any span inside .description-cell to have this style.
Add an explicit class name to the span. e.g. "status-description"

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:250
> +                content: ':';

A space after :?

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:252
> +            td.description-cell {

No need to say "td". There is no other element with that class name.

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:255
> +            div.warning-icon-container {

Ditto about div. But we shouldn't need this div at all.
Just absolutely position warning-icon element directly instead.

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:257
> +                top: -0.2rem;

This doesn't look right. Why do we need to shift by 0.2rem? Is that due to padding?

> Websites/perf.webkit.org/public/v3/components/test-group-revision-table.js:258
> +                left:0;

Nit: A space after ":".

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:340
> +        taskId = testGroup.task().id();

Why can't declare taskId here as const?

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:394
> +                {'wk': '191622', 'wk-patch': RemoteAPI.url('/api/uploaded-file/1.dat'), 'checkbox': 'build-wk', 'build-wk': true, 'build-request-id': '1', 'forcescheduler': 'force-ab-builds'}});

Nit: Wrong indentation. { should be exactly 4 spaces to the right of "assert".

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:26
> +        this._statusDescription = rawData['properties'] && rawData['properties']['statusdescription'] ? rawData['properties']['statusdescription'][0] : null;

Since statusdescription is not a standard Buildbot property, we should make this configurable.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:188
>                      } else if (!request.statusUrl()) {

We should just combine these two conditions to avoid code duplication.

Comment on attachment 374488 [details]
Patch

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

>> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:188
>>                      } else if (!request.statusUrl()) {
> 
> We should just combine these two conditions to avoid code duplication.

The updates are the same but the logging info are different.

[Ryosuke Niwa]

(In reply to dewei_zhu from comment #7)
> Comment on attachment 374488 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374488&action=review
> 
> >> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:188
> >>                      } else if (!request.statusUrl()) {
> > 
> > We should just combine these two conditions to avoid code duplication.
> 
> The updates are the same but the logging info are different.

I don’t think we need two distinct logging. We can just log both URL and description.

Comment on attachment 374488 [details]
Patch

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

>> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:26
>> +        this._statusDescription = rawData['properties'] && rawData['properties']['statusdescription'] ? rawData['properties']['statusdescription'][0] : null;
> 
> Since statusdescription is not a standard Buildbot property, we should make this configurable.

Used rawData['state_string'] instead as this is a standard property.

Landed in r250465.

rdar://problem/34941447