[Webkit-unassigned] [Bug 169542] Rewrite 'pull-os-versions' script in JavaScript and add ability to report os commits with sub-commits.

Tue Mar 14 22:48:54 PDT 2017

https://bugs.webkit.org/show_bug.cgi?id=169542

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #304475|review?                     |review+
              Flags|                            |

--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 304475
  --> https://bugs.webkit.org/attachment.cgi?id=304475
Patch

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

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:88
> +    function fetch_last_reported_between_orders($repository_id, $from=NULL, $to=NULL)

I don't think we should make $from and $to optional if the function name says between_orders.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:97
> +        $statements = 'SELECT commit_id as "id",
> +            commit_revision as "revision",
> +            commit_previous_commit as "previousCommit",
> +            commit_time as "time",
> +            commit_order as "order",
> +            committer_name as "authorName",
> +            committer_account as "authorEmail",
> +            commit_message as "message"

Instead of repeating these names, we can just fetch * and then call format_single_commit.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:100
> +        if ($from && $to) {

Get rid of this if.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:103
> +            $statements .= ' AND commit_order >=' . $from . ' AND commit_order <= ' . $to . ' ORDER BY commit_order DESC LIMIT 1';

Please use $1 and $2 instead.

> Websites/perf.webkit.org/public/include/commit-log-fetcher.php:112
> +        foreach ($commits as &$commit)
> +            $commit['time'] = Database::to_js_time($commit['time']);
> +

This is in fact not fetching committer information so this API is kind of broken.

> Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:301
> -    it("should distinguish between repositories with the asme name but with a different owner.", () => {
> +    it("should distinguish between repositories with the same name but with a different owner.", function () {

Keep the arrow function.

> Websites/perf.webkit.org/server-tests/resources/mock-subprocess.js:30
> +        var originalSubprocess = global.Subprocess;

Use const.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:35
> +        'WebKit': {
> +            'revision': '141978',
> +        }

Might wanna put this in a single line for simplicity.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:41
> +        'WebKit': {
> +            'revision': '141999',
> +        }

Ditto.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:50
> +        'WebKit': {
> +            'revision': '142000',
> +        },
> +        'JavaScriptCore': {
> +            'revision': '142000',
> +        }

Ditto.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:106
> +        it('should calculate the right order for a given valid revision', (done) => {

For sync tests, you don't need "done" at all.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:134
> +            const fetchCommitsPromise = fetchter._commitsForAvailableBuilds('OSX', ['list', 'build1'], '^\\.*$', 1604000000);

Please declare a local variable like "const minimumOrder = 1604000000" so that the number doesn't look as mysterious.

> Websites/perf.webkit.org/server-tests/tools-os-build-fetcher-tests.js:148
> +                assert.equal(results.length, 2);
> +                assert.equal(results[0]['repository'], 'OSX');
> +                assert.equal(results[0]['revision'], '16E321z');
> +                assert.equal(results[0]['order'], 1604032126);
> +                assert.equal(results[1]['repository'], 'OSX');
> +                assert.equal(results[1]['revision'], '16F321');
> +                assert.equal(results[1]['order'], 1605032100);

You might wanna just do deepEqual here.

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:23
> +        return Promise.all(customCommands.map(command => {

Nit: We usually wrap arguments in ().

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:29
> +            let fetchCommitsPromise = this._remoteAPI.getJSONWithStatus(`/api/commits/${escape(repositoryName)}/last-reported?from=${minRevisionOrder}&to=${maxRevisionOrder}`).then(result => {

I think we usually wrap argument in () for arrow functions as in (result) => {

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:45
> +        const buildNameRegex = /(\d+)([a-zA-z])(\d+)([a-zA-z]*)/;

It should be A-Z, not A-z. You might wanna even add a test!

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:71
> +        commits.forEach((commit) => {

I think it reads better to use "for (let commit of commits)"

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:75
> +                        commit['subCommits'] = JSON.parse(subCommitOutput);

I think we should verify that the content returned by the command in a good form.
We might wanna extract that as a separate function along with the call to _subprocess.execute.

And then use Array.prototype.reduce:
return commits.reduce((promise, commit) => {
    return promise.then(this._executeSubCommitCommand(command, commit['revision']));
}, Promise.resolve());

> Websites/perf.webkit.org/tools/js/os-build-fetcher.js:89
> +    fetchAndReportNewBuilds()

Again, please put this right after the constructor.

> Websites/perf.webkit.org/tools/js/subprocess.js:5
> +    call(command) {

We probably should not call this function "call" since we have https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call.
execute()?

> Websites/perf.webkit.org/tools/pull-os-versions.js:30
> +    // v3 models use the global RemoteAPI to access the perf dashboard.
> +    global.RemoteAPI = new RemoteAPI(serverConfig.server);

You can just get rid of the global RemoteAPI here.
I don't any code uses it.

> Websites/perf.webkit.org/tools/pull-os-versions.js:34
> +    Promise.all(
> +        osConfigList.map(osConfig => new OSBuildFetcher(osConfig, global.RemoteAPI, new Subprocess, serverConfig.slave, console))
> +    ).catch(function (error) {

I think it reads better to put these three lines into one.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170315/4b9766b6/attachment-0001.html>