[webkit-reviews] review granted: [Bug 199810] Provide build request status description information on dashboard. : [Attachment 374488] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 199810: Provide build request status description information on dashboard.
https://bugs.webkit.org/show_bug.cgi?id=199810

Attachment 374488: Patch

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




--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 374488
  --> https://bugs.webkit.org/attachment.cgi?id=374488
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.