Bug 176526 – Add support for builderNameToIDMap in BuildbotSyncer

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

As part of supporting Buildbot 0.9, we will need to fetch builderName to ID mapping. We should add support to handle this mapping in existing perf syncing scripts (without breaking any of the current functionality).

[Aakash Jain]

Created attachment 320117 [details]
Proposed patch

[Ryosuke Niwa]

Comment on attachment 320117 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320117&action=review

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:62
> +        assert(builderNameToIDMap);

On my second thought, I don't think it makes much sense for BuildbotSyncer itself to have the map.
Each syncer can be associated with exactly one builder so we should instead add object.builderID or something.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:321
>              if (!builderSyncer) {
> -                builderSyncer = new BuildbotSyncer(remote, builderInfo, commonConfigurations);
> +                builderSyncer = new BuildbotSyncer(remote, builderInfo, commonConfigurations, builderNameToIDMap);

A more important code to have here is to check that each builder we find appears in the mapping.
In fact, since one of the most common bugs we find in our configuration is that some builders in config json
are missing from buildbot, we should validate that each builder name appears in the map.

We should actually implement this validation for buildbot 0.8 using /json/builders/ for now.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:27
> +        this._builderNameToIDMap = {};

I don't think it makes sense for this map to be stored as an instance variable.
It's only used while constructing BuildbotSyncer objects.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:33
>          return new Promise((resolve, reject) => {

It should be a local variable here.
In fact, we should actually fetch /json/builders/ and verify that each builder name
that appear in the configuration file appears on buildbot.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:148
> +    return {}

We need to do more testing than just returning an empty dictionary.

[Ryosuke Niwa]

Comment on attachment 320117 [details]
Proposed patch

r- for reasons I've stated.

[Aakash Jain]

Created attachment 326308 [details]
Updated patch

Incorporated review comments.

> On my second thought, I don't think it makes much sense for BuildbotSyncer
> itself to have the map.
> Each syncer can be associated with exactly one builder so we should instead
> add object.builderID or something.

Done. This information is now in object.builderID.


> A more important code to have here is to check that each builder we find appears in the mapping.
> In fact, since one of the most common bugs we find in our configuration is
> that some builders in config json are missing from buildbot, we should validate that each builder name appears
> in the map.
> 
> We should actually implement this validation for buildbot 0.8 using
> /json/builders/ for now.

Implemented this validation, by fetching the builder list from buildbot 0.8 using /json/builders/ (for now) and asserting that the builder we found in config is present in Buildbot mapping.

[Aakash Jain]

Created attachment 326645 [details]
Updated patch

Updated server-tests.

Added code to fetch builderNameToIDMap from both Buildbot 0.8 and 0.9. Currently using Buildbot 0.8 format. Switching to Buildbot 0.9 would be simple later on.

[Aakash Jain]

Created attachment 327001 [details]
Updated patch

[Ryosuke Niwa]

Comment on attachment 327001 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327001&action=review

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:152
> +        assert.equal(MockRemoteAPI.requests[0].method, 'GET');
> +        assert.equal(MockRemoteAPI.requests[0].url, MockData.buildbotBuildersURL());
> +        MockRemoteAPI.requests[0].resolve(MockData.mockBuildbotBuilders());

Use assertAndResolveRequest?

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:252
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Where did request 0 go? I need the answer to this question before I can r+ this patch.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:365
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Again, where did request 0 go?

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:636
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:923
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:995
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1080
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1187
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1484
> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());

Ditto.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:21
> +    buildbot09BuildersURL() {return '/api/v2/builders'},

This is not used anywhere. We shouldn't add it until it's actually used.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:248
> +    mockBuildbot09Builders: function ()

Ditto.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:31
> +    getBuilderNameToIDMap() {

Nit: { should be on a new line.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:44
> +    getBuilderNameToIDMapDeprecated() {

Nit: { should be on its own line.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:49
> +            for (let builder in content) {
> +                builderNameToIDMap[builder] = builder;
> +            }

Nit: No curly braces around a single line statement.

[Aakash Jain]

Comment on attachment 327001 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327001&action=review

>> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:152
>> +        MockRemoteAPI.requests[0].resolve(MockData.mockBuildbotBuilders());
> 
> Use assertAndResolveRequest?

assertAndResolveRequest is defined later on inside describe(). Not available here. 

Also, this is the reason for missing requests[0] in most of the test-cases. All the methods calling createTriggerable() have the requests[0] here.

>> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:252
>> +            assertAndResolveRequest(requests[1], 'GET', MockData.buildbotBuildersURL(), MockData.mockBuildbotBuilders());
> 
> Where did request 0 go? I need the answer to this question before I can r+ this patch.

request[0] is inside createTriggerable(). I can add another assertAndResolveRequest for requests[0] here (just after createTriggerable) if that improves readability.

>> Websites/perf.webkit.org/server-tests/resources/mock-data.js:21
>> +    buildbot09BuildersURL() {return '/api/v2/builders'},
> 
> This is not used anywhere. We shouldn't add it until it's actually used.

It would be used when we do the final switch. This (and mockBuildbotBuilders below) will help in making the final patch for switch small.

I have tested that the server-tests pass with these data.

>> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:31
>> +    getBuilderNameToIDMap() {
> 
> Nit: { should be on a new line.

will take care of all these formatting issues.

[Ryosuke Niwa]

Comment on attachment 327001 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327001&action=review

>>> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:152
>>> +        MockRemoteAPI.requests[0].resolve(MockData.mockBuildbotBuilders());
>> 
>> Use assertAndResolveRequest?
> 
> assertAndResolveRequest is defined later on inside describe(). Not available here. 
> 
> Also, this is the reason for missing requests[0] in most of the test-cases. All the methods calling createTriggerable() have the requests[0] here.

Then we should move it move it outside describe() and use it here.
Also, we should add a new method to MockRemoteAPI (inside unit-tests/resources/mock-remote-api.js) like clear()
which clears the requests so that each doesn't need to shift its expectations by 1 everywhere as seen below.

[Aakash Jain]

Created attachment 328622 [details]
Updated patch

> Then we should move it move it outside describe() and use it here.
Done.

> Also, we should add a new method to MockRemoteAPI
MockRemoteAPI already have a method "reset" which does exactly this. Used MockRemoteAPI.reset(). 

Thanks for the suggestion. It simplified the server-tests changes significantly.

[Aakash Jain]

Comment on attachment 328622 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328622&action=review

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:754
>                  syncPromise = triggerable.initSyncers().then(() => triggerable.syncOnce());

This test calls initSyncers() twice. Calling MockRemoteAPI.reset() here would still affect the indexes of the subsequent asserts. Therefore indexes of subsequent requests are increased by 1 in this test.

[Ryosuke Niwa]

Comment on attachment 328622 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328622&action=review

r=me provided buildbot09BuildersURL and mockBuildbot09Builders are removed, and tests are added for getBuilderNameToIDMap.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:21
> +    buildbot09BuildersURL() {return '/api/v2/builders'},

Again, I don't think we should add these since we're not simultaneously supporting both 0.8 and 0.9.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:248
> +    mockBuildbot09Builders: function ()

Ditto.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:31
> +    getBuilderNameToIDMap()

Add a test for this function since it's not used.

[Aakash Jain]

Created attachment 331700 [details]
Updated Patch

Added test for getBuilderNameToIDMap() in server-tests/tools-buildbot-triggerable-tests.js

This new test needs buildbot09BuildersURL and mockBuildbot09Builders from mock-data, so I have to keep them.

[Ryosuke Niwa]

Comment on attachment 331700 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=331700&action=review

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:21
> +    buildbotBuildersURL() {return '/json/builders'},
> +    buildbot09BuildersURL() {return '/api/v2/builders'},

Please rename to buildbotBuildersURLDeprecated and buildbotBuildersURL.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:225
> +    mockBuildbotBuilders: function ()

Let's called this mockBuildbotBuildersDeprecated.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:248
> +    mockBuildbot09Builders: function ()

Let's call this mockBuildbotBuilders

[Aakash Jain]

Created attachment 331703 [details]
Patch for landing

Renamed variables as suggested above.

[Aakash Jain]

Committed r227184: <https://trac.webkit.org/changeset/227184>

[Radar WebKit Bug Importer]

<rdar://problem/36645842>