As part of supporting Buildbot 0.9, BuildbotBuildEntry (inside OpenSource/Websites/perf.webkit.org/tools/js/buildbot-syncer.js) should handle the Buildbot 0.9 data format.
Created attachment 332121 [details] Proposed patch Created new BuildbotBuildEntry class and moved previous one to BuildbotBuildEntryDeprecated. Did not sub-classed (or did not tried to share code between these classes) as we are not simultaneously supporting both Buildbot versions. BuildbotBuildEntryDeprecated will be simply deleted later on.
Comment on attachment 332121 [details] Proposed patch Attachment 332121 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6191128 New failing tests: http/tests/preload/onerror_event.html http/tests/misc/bubble-drag-events.html
Created attachment 332125 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 332565 [details] Proposed patch
Comment on attachment 332565 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=332565&action=review > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:38 > + buildRequestStatusIfUpdateIsNeeded(request) We shouldn't be duplicating the code for this function. r- due to this code duplication. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:68 > + this._isPending = false; > + if ('claimed' in rawData) > + this._isPending = !rawData['claimed']; These three lines of code should just be: this._isPending = 'claimed' in rawData && !rawData['claimed']. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:75 > + if (!('properties' in rawData)) > + return; > + if (rawData.properties.workername) > + this._workerName = rawData.properties.workername[0]; These lines code should simply be: this._workerName = rawData['properties'] && rawData['properties']['workername'] ? rawData['properties']['workername'][0] : null. But is it really guaranteed that worker name always contain exactly one item? > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:78 > + syncer() { return this._syncer; } Instead of duplicating all the code, make one inherit from another. > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:271 > + urlForPendingBuild(buildrequestid) { return this._remote.url(`/#/buildrequests/${buildrequestid}`); } buildrequestid should be camelCase: buildRequestId
Created attachment 332927 [details] Updated patch with unit-tests Added unit-tests. Removed code-duplication by inheriting BuildbotBuildEntryDeprecated from BuildbotBuildEntry. Incorporated review comments.
Comment on attachment 332927 [details] Updated patch with unit-tests View in context: https://bugs.webkit.org/attachment.cgi?id=332927&action=review > Websites/perf.webkit.org/tools/js/buildbot-syncer.js:253 > + urlForPendingBuild(buildRequestId) { return this._remote.url(`/#/buildrequests/${buildRequestId}`); } Huh, it's neat that you can find a build based on a build property. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1278 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let buildbotData = samplePendingBuild(); > + let pendingEntries = buildbotData.buildrequests.map((entry) => new BuildbotBuildEntry(syncer, entry)); Use const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1281 > + let entry = pendingEntries[0]; Ditto. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1298 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let buildbotData = sampleInProgressBuild(); > + let entries = buildbotData.builds.map((entry) => new BuildbotBuildEntry(syncer, entry)); > + > + assert.equal(entries.length, 1); > + let entry = entries[0]; Ditto for using const. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1315 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let buildbotData = sampleFinishedBuild(); > + let entries = buildbotData.builds.map((entry) => new BuildbotBuildEntry(syncer, entry)); > + > + assert.deepEqual(entries.length, 1); > + let entry = entries[0]; Ditto. > Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1329 > + let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI, sampleiOSConfig(), builderNameToIDMap())[1]; > + let buildbotData = {'builds': [sampleInProgressBuildData(), sampleFinishedBuildData()]}; > + let entries = buildbotData.builds.map((entry) => new BuildbotBuildEntry(syncer, entry)); Ditto.
Created attachment 332939 [details] Patch for landing Used const instead of let.
Comment on attachment 332939 [details] Patch for landing Clearing flags on attachment: 332939 Committed r227996: <https://trac.webkit.org/changeset/227996>
All reviewed patches have been landed. Closing bug.
<rdar://problem/37146332>