Bug 182036 – Create BuildbotBuildEntry in Buildbot syncer in Buildbot 0.9 format

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

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.

[Aakash Jain]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Aakash Jain]

Created attachment 332565 [details]
Proposed patch

[Ryosuke Niwa]

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

[Aakash Jain]

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

Added unit-tests.
Removed code-duplication by inheriting BuildbotBuildEntryDeprecated from BuildbotBuildEntry.
Incorporated review comments.

[Ryosuke Niwa]

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.

[Aakash Jain]

Created attachment 332939 [details]
Patch for landing

Used const instead of let.

[WebKit Commit Bot]

Comment on attachment 332939 [details]
Patch for landing

Clearing flags on attachment: 332939

Committed r227996: <https://trac.webkit.org/changeset/227996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37146332>