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).
Created attachment 320117 [details] Proposed patch
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.
Comment on attachment 320117 [details] Proposed patch r- for reasons I've stated.
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.
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.
Created attachment 327001 [details] Updated patch
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.
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.
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.
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.
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.
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.
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.
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
Created attachment 331703 [details] Patch for landing Renamed variables as suggested above.
Committed r227184: <https://trac.webkit.org/changeset/227184>
<rdar://problem/36645842>