Bug 176125 - Add initSyncers method in BuildbotTriggerable
Summary: Add initSyncers method in BuildbotTriggerable
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks: 175969
  Show dependency treegraph
 
Reported: 2017-08-30 14:25 PDT by Aakash Jain
Modified: 2017-11-16 00:34 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (17.40 KB, patch)
2017-08-30 14:32 PDT, Aakash Jain
rniwa: review+
Details | Formatted Diff | Diff
Patch for landing (16.34 KB, patch)
2017-09-06 15:56 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2017-08-30 14:25:34 PDT
This is part of changes for https://bugs.webkit.org/show_bug.cgi?id=175969.

We should create a separate initSyncers() method in BuildbotTriggerable, which would initialize all BuildbotSyncer. This method could later be updated to support Buildbot 0.9.
Comment 1 Aakash Jain 2017-08-30 14:32:09 PDT
Created attachment 319404 [details]
Proposed patch
Comment 2 Ryosuke Niwa 2017-09-05 00:56:50 PDT
Comment on attachment 319404 [details]
Proposed patch

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

> Websites/perf.webkit.org/server-tests/tools-buildbot-triggerable-tests.js:40
> +                syncPromise = triggerable.initSyncers().then(() => {
> +                    return triggerable.syncOnce();
> +                });

Please just do:
triggerable.initSyncers().then(() => triggerable.syncOnce())
here and elsewhere. There's no need in adding the boilerplate of { return ~ }.

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:35
> +        return new Promise((resolve, reject) => {
> +            this._syncers = BuildbotSyncer._loadConfig(this._buildbotRemote, this._config);
> +            resolve();
> +        });

This would resolve the promise right away. In order to test your change, we need to do something like:
setTimeout(resolve, 0);
or:
Promise.resolve().then(() => this._syncers = BuildbotSyncer._loadConfig(this._buildbotRemote, this._config))
Comment 3 Ryosuke Niwa 2017-09-05 00:57:07 PDT
Comment on attachment 319404 [details]
Proposed patch

r=me provided those two issues are addressed.
Comment 4 Aakash Jain 2017-09-06 15:56:48 PDT
Created attachment 320071 [details]
Patch for landing
Comment 5 WebKit Commit Bot 2017-09-06 21:23:38 PDT
Comment on attachment 320071 [details]
Patch for landing

Clearing flags on attachment: 320071

Committed r221717: <http://trac.webkit.org/changeset/221717>
Comment 6 WebKit Commit Bot 2017-09-06 21:23:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2017-09-27 12:48:07 PDT
<rdar://problem/34694048>