Provide build request stauts detail information on dashboard.
Created attachment 374155 [details] Patch
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
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.
(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