Bug 155814 - Add a model for parsing buildbot JSON with unit tests
Summary: Add a model for parsing buildbot JSON with unit tests
Alias: None
Product: WebKit
Classification: Unclassified
Component: Perf Dashboard (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
Depends on:
Reported: 2016-03-23 15:48 PDT by Ryosuke Niwa
Modified: 2016-03-23 20:25 PDT (History)
3 users (show)

See Also:

Adds the classes (35.51 KB, patch)
2016-03-23 15:57 PDT, Ryosuke Niwa
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2016-03-23 15:48:29 PDT
In order to improve our A/B testing infrastructure, add a new model class for parsing JSON fetched from buildbot.
Comment 1 Ryosuke Niwa 2016-03-23 15:57:10 PDT
Created attachment 274788 [details]
Adds the classes
Comment 2 Joseph Pecoraro 2016-03-23 20:18:43 PDT
Comment on attachment 274788 [details]
Adds the classes

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


> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:14
> +        this._isInProgress = 'currentStep' in rawData;

This state never changes and is never checked. Will it eventually?

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:112
> +            }

Nit: Semicolon.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:157
> +                assert.equal(typeof(value), 'object', `arguments should be a dictionary`);

Nit: Assert message here could be a string instead of a template string.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:165
> +                for (let part of value)
> +                    assert.equal(typeof(part), 'string', 'test should be an array of strings')

You could assert value.every((function (part) { return typeof part == 'string'; }). Whatever suits your style.

> Websites/perf.webkit.org/tools/js/buildbot-syncer.js:206
> +                assert(false, `Unrecognized named arguemnt ${keys[0]}`);

Typo: "arguemnt"

> Websites/perf.webkit.org/unit-tests/buildbot-syncer-tests.js:3
> +let assert = require('assert');

I see you are starting to use `let`!
Comment 3 Ryosuke Niwa 2016-03-23 20:25:09 PDT
Committed r198614: <http://trac.webkit.org/changeset/198614>