WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
199810
Provide build request status description information on dashboard.
https://bugs.webkit.org/show_bug.cgi?id=199810
Summary
Provide build request status description information on dashboard.
dewei_zhu
Reported
2019-07-15 14:59:52 PDT
Provide build request stauts detail information on dashboard.
Attachments
Patch
(44.38 KB, patch)
2019-07-15 15:32 PDT
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(44.89 KB, patch)
2019-07-18 23:40 PDT
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(45.31 KB, patch)
2019-07-18 23:45 PDT
,
dewei_zhu
no flags
Details
Formatted Diff
Diff
Patch
(46.20 KB, patch)
2019-07-19 12:10 PDT
,
dewei_zhu
rniwa
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
dewei_zhu
Comment 1
2019-07-15 15:32:32 PDT
Created
attachment 374155
[details]
Patch
Ryosuke Niwa
Comment 2
2019-07-17 17:27:13 PDT
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.
dewei_zhu
Comment 3
2019-07-18 23:40:46 PDT
Created
attachment 374445
[details]
Patch
dewei_zhu
Comment 4
2019-07-18 23:45:58 PDT
Created
attachment 374446
[details]
Patch
dewei_zhu
Comment 5
2019-07-19 12:10:15 PDT
Created
attachment 374488
[details]
Patch
Ryosuke Niwa
Comment 6
2019-07-23 21:43:53 PDT
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.
dewei_zhu
Comment 7
2019-07-24 08:51:20 PDT
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
Comment 8
2019-07-24 11:07:29 PDT
(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.
dewei_zhu
Comment 9
2019-09-27 15:25:38 PDT
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.
dewei_zhu
Comment 10
2019-09-27 15:45:58 PDT
Landed in
r250465
.
dewei_zhu
Comment 11
2019-09-30 11:26:28 PDT
rdar://problem/34941447
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug