Bug 179743

Summary: Add support for fetching recent builds in Buildbot 0.9 format in BuildbotSyncer
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, dean_johnson, dewei_zhu, ews-watchlist, lforschler, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=182036
https://bugs.webkit.org/show_bug.cgi?id=182218
https://bugs.webkit.org/show_bug.cgi?id=175969
Attachments:
Description Flags
Proposed patch
none
Proposed patch
rniwa: review-
Updated patch
rniwa: review-
Updated patch with unit-tests
rniwa: review+
Archive of layout-test-results from ews101 for mac-sierra
none
Patch for landing none

Aakash Jain
Reported 2017-11-15 13:48:16 PST
As part of supporting Buildbot 0.9, BuildbotSyncer should be able to fetch recent builds from Buildbot 0.9 server. We should add support to handle this in existing perf syncing scripts without breaking any of the current functionality.
Attachments
Proposed patch (2.80 KB, patch)
2017-11-15 13:52 PST, Aakash Jain
no flags
Proposed patch (3.41 KB, patch)
2018-01-22 21:11 PST, Aakash Jain
rniwa: review-
Updated patch (3.34 KB, patch)
2018-01-30 23:12 PST, Aakash Jain
rniwa: review-
Updated patch with unit-tests (7.82 KB, patch)
2018-02-02 11:09 PST, Aakash Jain
rniwa: review+
Archive of layout-test-results from ews101 for mac-sierra (2.32 MB, application/zip)
2018-02-02 11:51 PST, EWS Watchlist
no flags
Patch for landing (7.70 KB, patch)
2018-02-05 07:50 PST, Aakash Jain
no flags
Aakash Jain
Comment 1 2017-11-15 13:52:22 PST
Created attachment 327021 [details] Proposed patch
Aakash Jain
Comment 2 2018-01-22 21:11:20 PST
Created attachment 332004 [details] Proposed patch
Ryosuke Niwa
Comment 3 2018-01-30 00:22:12 PST
Comment on attachment 332004 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=332004&action=review We need tests for this. r- due to the lack of tests. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:219 > + const entries = []; > + for (let build of content.builds) > + entries.push(new BuildbotBuildEntry(this, build)); > + return entries; There is no point in manually pushing. Just do: return content.builds.map((build) => new BuildbotBuildEntry(this, build)); > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:224 > + pathForRecentBuilds(count) { return `/api/v2/builders/${this._builderID}/builds?` + `limit=${count}&order=-number&property=*`; } What why `+ ` between "builds?" and "limit="? Please concatenate them instead.
Aakash Jain
Comment 4 2018-01-30 23:12:38 PST
Created attachment 332744 [details] Updated patch > There is no point in manually pushing. Just do: > return content.builds.map((build) => new BuildbotBuildEntry(this, build)); Done. Thanks for the suggestion. > What why `+ ` between "builds?" and "limit="? Please concatenate them instead. Done > We need tests for this. I have tried to break down overall changes into smaller patches. For e.g. this patch add code to just fetch the recent builds (but does not enable this code path yet). Final patch in https://bugs.webkit.org/show_bug.cgi?id=175969 start using all this code and all the test-cases (unit-tests and server-tests) are updated in that.
Ryosuke Niwa
Comment 5 2018-01-31 12:21:51 PST
(In reply to Aakash Jain from comment #4) > Created attachment 332744 [details] > Updated patch > > > There is no point in manually pushing. Just do: > > return content.builds.map((build) => new BuildbotBuildEntry(this, build)); > Done. Thanks for the suggestion. > > > What why `+ ` between "builds?" and "limit="? Please concatenate them instead. > Done > > > We need tests for this. > > I have tried to break down overall changes into smaller patches. For e.g. > this patch add code to just fetch the recent builds (but does not enable > this code path yet). Final patch in > https://bugs.webkit.org/show_bug.cgi?id=175969 start using all this code and > all the test-cases (unit-tests and server-tests) are updated in that. We need to start testing this code right now, not in the final patch. The whole point of breaking down patches was to make each patch reviewable. And that includes tests for those broken up pieces.
Ryosuke Niwa
Comment 6 2018-01-31 12:22:05 PST
Comment on attachment 332744 [details] Updated patch Again, we need a test for this. r-
Aakash Jain
Comment 7 2018-02-02 11:09:04 PST
Created attachment 332983 [details] Updated patch with unit-tests Added unit-tests as discussed.
EWS Watchlist
Comment 8 2018-02-02 11:51:06 PST Comment hidden (obsolete)
EWS Watchlist
Comment 9 2018-02-02 11:51:07 PST Comment hidden (obsolete)
Ryosuke Niwa
Comment 10 2018-02-02 18:36:08 PST
Comment on attachment 332983 [details] Updated patch with unit-tests View in context: https://bugs.webkit.org/attachment.cgi?id=332983&action=review > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1356 > + it('should not fetch recent builds when count is zero', () => { Use async "() => {" here > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1357 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; Use const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1362 > + return promise.then((content) => { > + assert.deepEqual(content, []); > + }); and do: await promise; assert.deepEqual(content, []); i.e. async () => { const syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; const promise = syncer._pullRecentBuilds(0); assert.equal(requests.length, 0); await promise; assert.deepEqual(content, []); } > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1366 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; Use const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1374 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let promise = syncer._pullRecentBuilds(2); Use const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1380 > + return promise.then((content) => { > + assert.deepEqual(content, []); > + }); Ditto about using async await. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1383 > + it('should create BuildbotBuildEntry after fetching recent builds', () => { Use async > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1385 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let promise = syncer._pullRecentBuilds(2); use const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1391 > + return promise.then((entries) => { > + assert.deepEqual(entries.length, 2); Use await.
Aakash Jain
Comment 11 2018-02-05 07:50:20 PST
Created attachment 333081 [details] Patch for landing Incorporated review comments.
Aakash Jain
Comment 12 2018-02-05 08:54:35 PST
Radar WebKit Bug Importer
Comment 13 2018-02-05 08:55:28 PST
Note You need to log in before you can comment on or make changes to this bug.