Bug 155921 – BuildbotSyncer should be able to fetch JSON from buildbot

• Status: RESOLVED
• Resolution: FIXED
• Component: Perf Dashboard

[Ryosuke Niwa]

Add a method to fetch pending, in-progress, and finished build information from buildbot.

[Ryosuke Niwa]

Created attachment 274976 [details]
Adds a method

[Joseph Pecoraro]

Comment on attachment 274976 [details]
Adds a method

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

r=me

> Websites/perf.webkit.org/ChangeLog:9
> +        with lots of unit tests as this has historically been a source of of subtle bugs in the old script.

Typo: "of of" => "of"

> Websites/perf.webkit.org/ChangeLog:11
> +        New implementation fixes a subtle in the old pythons script which overlooked the possibility that the

Grammar: "fixes a subtle in"

> Websites/perf.webkit.org/ChangeLog:14
> +        a pending may start running or an in-progress build finish and shift the offset by one. Thew new script

Grammar: "a pending may start" => "a pending build may start"
Typo: "Thew" => "The"

> Websites/perf.webkit.org/ChangeLog:15
> +        avoids this problem by requesting all previous builds by single HTTP request with multiple select query

Grammar: "by single" => "by a single"

> Websites/perf.webkit.org/ChangeLog:16
> +        parameters after requesting pending builds, and then letting pending requests overridden by in-progress

Grammar: "then letting pending requests overridden"

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:86
> +            var entries = [];

Nit: `let` instead of `var`

> Websites/perf.webkit.org/tools/js/v3-models.js:35
> +global['RemoteAPI'] = require('./remote.js').RemoteAPI;
> +
>  global.Statistics = require('../../public/shared/statistics.js');

Nit: Two different styles. "global.RemoteAPI" would be simpler.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:474
> +            let promise = syncer.pullBuildbot(1);

This local variable is not used. Should it be? Otherwise, you can drop it.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:524
> +            Promise.resolve().then(function (entries) {

You probably don't want the parameter "entries" here. There are bunch of these "Promise.resolve().then(function (xxx) {" lines where the "xxx" portion can be dropped.

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:620
> +        it('should overrde BuildbotBuildEntry for pending builds by in-progress builds', function (done) {

Typo: "overdue" => "override"

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:650
> +        it('should overrde BuildbotBuildEntry for pending builds by finished builds', function (done) {

Typo: "overdue" => "override"

[Ryosuke Niwa]

Thanks for the review! Will address the comment & land.

[Ryosuke Niwa]

Committed r198824: <http://trac.webkit.org/changeset/198824>