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.
Created attachment 327021 [details] Proposed patch
Created attachment 332004 [details] Proposed patch
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.
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.
(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.
Comment on attachment 332744 [details] Updated patch Again, we need a test for this. r-
Created attachment 332983 [details] Updated patch with unit-tests Added unit-tests as discussed.
Comment on attachment 332983 [details] Updated patch with unit-tests Attachment 332983 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6334534 New failing tests: media/modern-media-controls/tracks-support/tracks-support-show-panel-after-dragging-controls.html
Created attachment 332987 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
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.
Created attachment 333081 [details] Patch for landing Incorporated review comments.
Committed r228098: <https://trac.webkit.org/changeset/228098>
<rdar://problem/37238352>