Bug 179743 – Add support for fetching recent builds in Buildbot 0.9 format in BuildbotSyncer

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

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.

[Aakash Jain]

Created attachment 327021 [details]
Proposed patch

[Aakash Jain]

Created attachment 332004 [details]
Proposed patch

[Ryosuke Niwa]

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]

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]

(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 on attachment 332744 [details]
Updated patch

Again, we need a test for this. r-

[Aakash Jain]

Created attachment 332983 [details]
Updated patch with unit-tests

Added unit-tests as discussed.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

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]

Created attachment 333081 [details]
Patch for landing

Incorporated review comments.

[Aakash Jain]

Committed r228098: <https://trac.webkit.org/changeset/228098>

[Radar WebKit Bug Importer]

<rdar://problem/37238352>