[webkit-reviews] review granted: [Bug 182218] Add support for submitting build request to Buildbot 0.9 server in BuildbotSyncer : [Attachment 332999] Updated patch with unit-test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Aakash Jain
<aakash_jain at apple.com>'s request for review:
Bug 182218: Add support for submitting build request to Buildbot 0.9 server in
BuildbotSyncer
https://bugs.webkit.org/show_bug.cgi?id=182218

Attachment 332999: Updated patch with unit-test

https://bugs.webkit.org/attachment.cgi?id=332999&action=review




--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 332999
  --> https://bugs.webkit.org/attachment.cgi?id=332999
Updated patch with unit-test

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

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:143
> +	   assert(properties['forcescheduler'], "forcescheduler must be defined
for builder: " + this._builderName + " in order to post a build request to
Buildbot.")

This is not an accurate error message. We must specify a forcebuilder for a
given request, not a builder.
Revise this to:
`forcescheduler was not specified in buildbot properties for build request
${newRequest.id()} on platform "${newRequest.platform().name()}" for builder
"${this.builderName()}"`;

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:262
> +    pathForForceBuild(scheduler) { return
`/api/v2/forceschedulers/${scheduler}`; }

This argument should be named schedulerName to signify the fact it's a string.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1620
> +	   it('should schedule a build request on Buildbot', () => {

Use async.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1621
> +	       let syncer = BuildbotSyncer._loadConfig(MockRemoteAPI,
sampleiOSConfig(), builderNameToIDMap())[0];

Use const.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:1627
> +	       return waitForRequest.then(() => {

Use await.