Update buildbot-syncer.js and related scripts inside Websites/perf.webkit.org/tools for Buildbot 0.9.
Created attachment 319050 [details] Proposed patch This is still WIP. I have tested with staging Buildbot 0.9 instance and the changes seems to work fine. I am currently working on unit-tests. Would be nice to have the review comments meanwhile.
Created attachment 319059 [details] Updated patch Updated patch with some Ryosuke's feedback incorporated.
Comment on attachment 319059 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=319059&action=review > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:21 > + this._claimed = false; > + if (!('claimed' in rawData)) > + this._claimed = true; // This means this is build data, not build-request data. > + this._isInProgress = !rawData.complete && this._claimed; > + this._isPending = !rawData.complete && !this._claimed; > + this._buildNumber = rawData.number; claimed doesn't need to be stored as a member variable. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:74 > + if (object.properties) > + this._forceScheduler = object.properties.forcescheduler; We can't do this. properties are resolved in _propertiesForBuildRequest per each build request. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:79 > + forceScheduler() { return this._forceScheduler; } Don't add this public method. It's never called from outside this function. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:119 > + if (slaveName) > + this._slavesWithNewRequests.add(slaveName); This is wrong. It's important to insert "null" to _slavesWithNewRequests in the case slaveName isn't present. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:201 > + if (build) Why do we need to check for true-ness here? can build be null or something? Note that [] evaluates to true in JavaScript. All these random true-ness/false-ness check needs a test. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:209 > + pathForPendingBuildsJSON() { return `/api/v2/builders/${this.builderID()}/buildrequests?complete=false&claimed=false`; } > + pathForBuildJSON(count) { return `/api/v2/builders/${this.builderID()}/builds?` + `limit=${count}&order=-number&property=*`; } Don't call method to get builderID in these functions. It's less readable than just calling this._builderID. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:210 > + pathForForceBuild() { return `/api/v2/forceschedulers/${this.forceScheduler()}`; } We can't have this because properties is resolved per request. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:213 > + url() { return this._remote.url(`/builders/${this.builderID()}/`); } > + urlForBuildNumber(number) { return this._remote.url(`/builders/${this.builderID()}/builds/${number}`); } Ditto about not calling builderID > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:224 > + _dataForBuildRequest(properties) > + { > + const data = {}; > + data['jsonrpc'] = '2.0'; > + data['method'] = 'force'; > + data['id'] = properties[this._buildRequestPropertyName]; > + data['params'] = properties; > + data['params']['owner'] = 'Perf'; > + return data; > } We don't need this function since it's only called at once. A better way to write this code would be: const data = {jsonrpc: '2.0', method: 'force', id: properties[this._buildRequestPropertyName], params: properties} We don't need to set 'owner', do we? Just don't set it. > Websites/perf.webkit.org/tools/js/remote.js:-60 > - postFormUrlencodedData(path, data) You can't make this change to NodeRemoteAPI. It's used for other things during testing. Use RemoteAPI.postJSON defined in the abstract superclass instead: https://trac.webkit.org/browser/webkit/trunk/Websites/perf.webkit.org/public/shared/common-remote.js#L4
Created attachment 332625 [details] WIP Patch to switch to Buildbot 0.9. It will apply to ToT after landing https://bugs.webkit.org/show_bug.cgi?id=179743, https://bugs.webkit.org/show_bug.cgi?id=182036 and https://bugs.webkit.org/show_bug.cgi?id=182218 Code changes are minimal, mostly switching from Deprecated versions of classes/functions to new versions. Unit-tests and Server-tests are changed significantly as they start exercising new code. They still need some more update.
Created attachment 332743 [details] Patch to switch to Buildbot 0.9 - WIP
Created attachment 332782 [details] Updated Patch to switch to Buildbot 0.9 - WIP
Created attachment 332791 [details] Patch to switch to Buildbot 0.9 Please review. All server-tests and unit-tests pass except one unit-test regarding which I have a question. Buildbot 0.9 api doesn't seems to provide properties information for pending builds (buildrequests). So we don't get slavename information for pending build. Would the syncing script work fine without that info?
Created attachment 333309 [details] Updated patch Updated patch. All unit-tests and server-tests pass. Also tested with a Buildbot 0.9 instance.
Comment on attachment 333309 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=333309&action=review > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:-1811 > - it('should schedule a build if builder only has a pending build on a non-maching slave', () => { @Ryosuke, we discussed in-person about this last week. Buildbot 0.9 api doesn't provide slave-name information for pending builds. This currently doesn't affect us since we currently have one slave per builder, however we want to fix it. We decided to add a constraint to syncing script to not schedule a build if a builder has a pending build (irrespective of which slave it is on). Also, you volunteered to do that change.
Comment on attachment 333309 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=333309&action=review > Websites/perf.webkit.org/server-tests/resources/mock-data.js:23 > + pendingBuildsUrl: function (builderId) { return `/api/v2/builders/${builderId}/buildrequests?complete=false&claimed=false`; }, > + recentBuildsUrl: function (builderId, count) { return `/api/v2/builders/${builderId}/builds?limit=${count}&order=-number&property=*`; }, Passing builderId makes each test case impossible to follow. Please take the builder name here, and convert it to each ID in mockBuildbotBuilders.
Comment on attachment 333309 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=333309&action=review > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:216 > + return this._remote.getJSON(this.pathForPendingBuilds()).then((content) => { > + let pendingEntries = []; > + if ('buildrequests' in content) > + pendingEntries = content.buildrequests.map((entry) => new BuildbotBuildEntry(this, entry)); > + return this._pullRecentBuilds(count).then((entries) => { A more succinct way of writing this code would be: const pendingEntries = (content.buildrequests || []).map((entry) => new BuildbotBuildEntry(this, entry)); > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:158 > + } Please add ; while we're at. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:163 > + return samplePendingBuild(null, null, "some builder"); samplePendingBuild is a really badly name function since it returns a dictionary with a list of build requests. We should rename it to samplePendingBuildRequests or something. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:186 > + return sampleInProgressBuild(); Ditto. This should be sampleInProgressBuild*s*. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:212 > + return sampleFinishedBuild(null, null, "some builder"); Ditto. This should be sampleFinishedBuild*s*.
Comment on attachment 333309 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=333309&action=review >> Websites/perf.webkit.org/server-tests/resources/mock-data.js:23 >> + recentBuildsUrl: function (builderId, count) { return `/api/v2/builders/${builderId}/builds?limit=${count}&order=-number&property=*`; }, > > Passing builderId makes each test case impossible to follow. > Please take the builder name here, and convert it to each ID in mockBuildbotBuilders. Sure. >> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:216 >> + return this._pullRecentBuilds(count).then((entries) => { > > A more succinct way of writing this code would be: > const pendingEntries = (content.buildrequests || []).map((entry) => new BuildbotBuildEntry(this, entry)); Sure. Thanks for the suggestion. >> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:158 >> + } > > Please add ; while we're at. sure. >> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:163 >> + return samplePendingBuild(null, null, "some builder"); > > samplePendingBuild is a really badly name function since it returns a dictionary with a list of build requests. > We should rename it to samplePendingBuildRequests or something. Agreed. Renaming it in a separate patch https://bugs.webkit.org/show_bug.cgi?id=183171 >> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:186 >> + return sampleInProgressBuild(); > > Ditto. This should be sampleInProgressBuild*s*. sampleInProgressBuild() returns a single build, so I think we can keep it as is. >> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:212 >> + return sampleFinishedBuild(null, null, "some builder"); > > Ditto. This should be sampleFinishedBuild*s*. sampleFinishedBuild() returns a single build, so I think we can keep it as is.
Created attachment 335513 [details] Updated patch Updated patch with review comments incorporated.
Created attachment 335979 [details] Updated patch rebased on TOT Rebased on TOT (after https://trac.webkit.org/r229618).
Comment on attachment 335979 [details] Updated patch rebased on TOT View in context: https://bugs.webkit.org/attachment.cgi?id=335979&action=review I'm done 2/3 of the way. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:61 > + assert.deepEqual(MockRemoteAPI.requests[2].data, {'id': 702, 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': 702, 'forcescheduler': 'force-some-builder-1'}}); Nit: Place { for the inner object in the new line. This patches our convention to always put +, ||, etc... at the beginning of the next line when wrapping lines. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:172 > + assert.deepEqual(MockRemoteAPI.requests[4].data, {'id': '700', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '700', 'forcescheduler': 'force-some-builder-2'}}); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:281 > - assert.equal(BuildRequest.findById(700).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/'); > + assert.equal(BuildRequest.findById(700).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Where did the number 17 come from? Is this the builder ID for some-builder-1? Then we should use some sort of a helper function to convert it from 17 instead. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:328 > + assert.deepEqual(MockRemoteAPI.requests[4].data, {'id': '702', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '702', 'forcescheduler': 'force-some-builder-1'}}); Ditto. Put { at the beginning of the new line. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:367 > - assert.equal(BuildRequest.findById(702).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/'); > + assert.equal(BuildRequest.findById(702).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto about converting from a human readable value to 17. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:475 > + assert.deepEqual(MockRemoteAPI.requests[4].data, {'id': '710', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '710', 'forcescheduler': 'force-some-builder-2'}}); Ditto about putting { on the new line. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:522 > - assert.equal(BuildRequest.findById(702).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/'); > + assert.equal(BuildRequest.findById(702).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto about converting some-builder-1 to 17 using a helper function. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:526 > - assert.equal(BuildRequest.findById(710).statusUrl(), 'http://build.webkit.org/builders/some%20builder%202/'); > + assert.equal(BuildRequest.findById(710).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:576 > + assert.deepEqual(MockRemoteAPI.requests[4].data, {'id': '701', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '701', 'forcescheduler': 'force-some-builder-2'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:581 > + assert.deepEqual(MockRemoteAPI.requests[5].data, {'id': '711', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '711', 'forcescheduler': 'force-some-builder-1'}}); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:591 > + MockRemoteAPI.requests[7].resolve(MockData.pendingBuild({builderId: 3, buildRequestId: 701})); Again, we should be getting 3 from a helper function. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:600 > + MockRemoteAPI.requests[9].resolve(MockData.runningBuild({builderId: 3, buildRequestId: 700})); Ditto. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:675 > + assert.deepEqual(requests[2].data, {'id': '700', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '700', 'forcescheduler': 'force-some-builder-1'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:698 > + assert.equal(BuildRequest.findById(700).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto about using the helper function to get 17.= > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:749 > + assert.deepEqual(MockRemoteAPI.requests[2].data, {'id': '701', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '701', 'forcescheduler': 'force-some-builder-1'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:817 > + assert.deepEqual(MockRemoteAPI.requests[2].data, {'id': '701', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '192736', 'os': '10.11 15A284', 'build-request-id': '701', 'forcescheduler': 'force-some-builder-1'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:848 > - assert.equal(BuildRequest.findById(701).statusUrl(), 'http://build.webkit.org/builders/some-builder-1/'); > + assert.equal(BuildRequest.findById(701).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto about using a helper function to get 17. > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:945 > + assert.deepEqual(MockRemoteAPI.requests[2].data, {'id': '710', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'os': '10.11 15A284', 'build-request-id': '710', 'forcescheduler': 'force-some-builder-1'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:260 > + assertAndResolveRequest(requests[0], 'GET', MockData.pendingBuildsUrl('some tester'), MockData.pendingBuild({builderId: 5, buildRequestId: 5})); Ditto about getting 5 from a helper function. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:266 > + MockData.runningBuild({builderId: 5, buildRequestId: 1})); Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:279 > + MockData.runningBuildData({builderId: 5, buildRequestId: 5}), > + MockData.finishedBuildData({builderId: 5, buildRequestId: 1})] Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:389 > + assert.deepEqual(requests[6].data, {'id': '1', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'wk-patch': RemoteAPI.url('/api/uploaded-file/1.dat'), 'checkbox': 'build-wk', 'build-wk': true, 'build-request-id': '1', 'forcescheduler': 'force-ab-builds'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:395 > + MockData.pendingBuild({builderId: 1, buildRequestId: 1})); Ditto about converting "some builder" to 1 using a helper function. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:402 > + MockData.runningBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:496 > + MockData.finishedBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:504 > + assert.deepEqual(requests[6].data, {'id': '2', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'build-request-id': '2', 'forcescheduler': 'force-ab-builds', 'checkbox': 'build-wk', 'build-wk': true}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:518 > + MockData.runningBuildData({builderId: 1, buildRequestId: 2}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1})] Ditto about getting builder ID 1 via a helper function. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:664 > + assert.deepEqual(requests[6].data, {'id': '1', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'wk-patch': RemoteAPI.url('/api/uploaded-file/1.dat'), 'build-request-id': '1', 'forcescheduler': 'force-ab-builds', Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:671 > + MockData.pendingBuild({builderId: 1, buildRequestId: 1})); Ditto about getting 1 using a helper function. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:678 > + MockData.runningBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:772 > + MockData.finishedBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:780 > + assert.deepEqual(requests[6].data, {'id': '2', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'build-request-id': '2', 'forcescheduler': 'force-ab-builds', 'checkbox': 'build-wk', 'build-wk': true, 'platform-name': 'some platform'}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:794 > + MockData.runningBuildData({builderId: 1, buildRequestId: 2}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1})] Ditto about getting 1 via a helper function. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:952 > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 2})] Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:960 > + assert.deepEqual(requests[6].data, {'id': '3', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'test': 'some-test', 'wk': '191622', 'build-request-id': '3','forcescheduler': 'force-ab-tests', 'roots': JSON.stringify([{url: firstRoot.url()}])}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:976 > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 2})] Ditto about using a helper function to get 1. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1027 > + MockData.runningBuildData({builderId: 1, buildRequestId: 2}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1044 > + MockData.runningBuildData({builderId: 1, buildRequestId: 2, buildNumber: 1002}), > + MockData.finishedBuildData({builderId: 1, buildRequestId: 1})] Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1116 > + ); Why are these trailing ) on a new line? Per our style guideline, they should all be on the previous line at the end of ). Please fix them all. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1228 > + assert.deepEqual(requests[6].data, {'id': '1', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'build-request-id': '1', 'forcescheduler': 'force-ab-builds', Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1353 > + assert.deepEqual(requests[6].data, {'id': '2', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '192736', 'build-request-id': '2', 'forcescheduler': 'force-ab-builds', 'owned-commits': `{"WebKit":[{"revision":"owned-jsc-9191","repository":"JavaScriptCore","ownerRevision":"192736"}]}`}}); Ditto about {. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1531 > + assert.deepEqual(requests[6].data, {'id': '1', 'jsonrpc': '2.0', 'method': 'force', 'params': { > + 'wk': '191622', 'build-request-id': '1', 'forcescheduler': 'force-ab-builds', 'owned-commits': `{"WebKit":[{"revision":"owned-jsc-6161","repository":"JavaScriptCore","ownerRevision":"191622"}]}`}}); Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1537 > + MockData.pendingBuild({builderId: 1, buildRequestId: 1})); Again, use a helper function to get 1. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1544 > + MockData.runningBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:1688 > + MockData.finishedBuild({builderId: 1, buildRequestId: 1}) Ditto. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:300 > + getBuilderIdForName: function(builderName) Per style guideline, we don't use the prefix of "get" unless there is an out parameter. Since this function doesn't have an out parameter, we should call this "builderIDForName" or "builderIDFromName" > Websites/perf.webkit.org/server-tests/resources/mock-data.js:307 > + const builderNameToIDMap = {}; > + for (const builder of builders) > + builderNameToIDMap[builder.name] = builder.builderid; > + > + return builderNameToIDMap[builderName]; Why are we building this map each time this function is getting called? We should just loop over the list and return once we find the first match.
Comment on attachment 335979 [details] Updated patch rebased on TOT View in context: https://bugs.webkit.org/attachment.cgi?id=335979&action=review > Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:207 > - assert.equal(BuildRequest.findById(700).statusUrl(), 'http://build.webkit.org/builders/some%20builder%202/'); > + assert.equal(BuildRequest.findById(700).statusUrl(), MockData.statusUrlForPendingBuilds(17)); Ditto about using a helper function for 17. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:22 > + pendingBuildsUrl: function (builderName) { return '/api/v2/builders/' + this.getBuilderIdForName(builderName) + '/buildrequests?complete=false&claimed=false&property=*'; }, Why don't we save the builder ID in a local variable and use a template literal for the entire URL for a better readability e.g. const builderId = this.getBuilderIdForName(builderName); return `/api/v2/builders/${builderId}/buildrequests?complete=false&claimed=false&property=*`; > Websites/perf.webkit.org/server-tests/resources/mock-data.js:24 > + recentBuildsUrl: function (builderName, count) { return '/api/v2/builders/' + this.getBuilderIdForName(builderName) + `/builds?limit=${count}&order=-number&property=*`; }, > + statusUrl: function (builderName, buildId) { return 'http://build.webkit.org/#/builders/' + this.getBuilderIdForName(builderName) + `/builds/${buildId}`; }, Ditto. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:313 > + "buildrequestid": 17, This should probably be configurable via options. In all the places where we hard core 17, we should probably specify 17 in pendingBuildData as well. And then in assertions, just hard code the entire string. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:369 > + "buildrequestid": 17, 17 should be configurable here. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:370 > + "complete": options.isComplete, Just do "options.isComplete || false" here. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:372 > + "buildid": 418744, buildid too should be configurable. > Websites/perf.webkit.org/server-tests/resources/mock-data.js:375 > + "started_at": 1513725109, Is this a POSIX time stamp? If so, can we just do: new Date('2017-12-19T23:11:49Z') / 1000 instead? > Websites/perf.webkit.org/server-tests/resources/mock-data.js:399 > + options = options || {}; > + options.isComplete = false; > + if (!options.buildRequestId) > + options.buildRequestId = 701; > + if (!options.buildNumber) > + options.buildNumber = 124; > + if (!options.webkitRevision) > + options.webkitRevision = '192736'; None of this code should be necessary once you add the fallback I suggested for "complete". > Websites/perf.webkit.org/server-tests/resources/mock-data.js:436 > + options = options || {}; > + options.isComplete = true; This will modify the original options if it's not null/undefined. That's not great. I think a better approach would be to add an override as an second argument to this.sampleBuildData instead. e.g. this.sampleBuildData(options, {isComplete: true}); and then do: "complete": 'isComplete' in overrides ? overrides.isComplete : (options.isComplete || false).
Comment on attachment 335979 [details] Updated patch rebased on TOT r=me assuming all my comments are addressed.
Comment on attachment 335979 [details] Updated patch rebased on TOT View in context: https://bugs.webkit.org/attachment.cgi?id=335979&action=review Incorporated all the review comments. >> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:281 >> + assert.equal(BuildRequest.findById(700).statusUrl(), MockData.statusUrlForPendingBuilds(17)); > > Where did the number 17 come from? Is this the builder ID for some-builder-1? > Then we should use some sort of a helper function to convert it from 17 instead. As discussed in-person, this is the buildrequestid generated by buildbot, we set 17 in our sample-data. Will pass it every unit-test while creating pendingBuild(). e.g.: MockData.pendingBuild({buildRequestId: 700, buildbotBuildRequestId: 17}
Committed r229728: <https://trac.webkit.org/changeset/229728>
<rdar://problem/38643063>