Bug 155921

Summary: BuildbotSyncer should be able to fetch JSON from buildbot
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: Perf DashboardAssignee: Ryosuke Niwa <rniwa>
Severity: Normal CC: cdumez, dewei_zhu, joepeck, kling, koivisto, rniwa, slewis
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Adds a method joepeck: review+

Description Ryosuke Niwa 2016-03-25 22:27:04 PDT
Add a method to fetch pending, in-progress, and finished build information from buildbot.
Comment 1 Ryosuke Niwa 2016-03-25 22:44:17 PDT
Created attachment 274976 [details]
Adds a method
Comment 2 Joseph Pecoraro 2016-03-29 13:37:28 PDT
Comment on attachment 274976 [details]
Adds a method

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


> 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"
Comment 3 Ryosuke Niwa 2016-03-29 20:08:31 PDT
Thanks for the review! Will address the comment & land.
Comment 4 Ryosuke Niwa 2016-03-29 20:16:25 PDT
Committed r198824: <http://trac.webkit.org/changeset/198824>