[Webkit-unassigned] [Bug 175978] Performance Dashboard backend should support A/B testing for owned components.

[bugzilla-daemon at webkit.org]

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

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

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

> Websites/perf.webkit.org/ChangeLog:12
> +        * init-database.sql: Updated 'commit_set_items' table to contain commit owner info and whether this commit requires build.
> +        SQL for existing database:
> +            'ALTER TABLE ADD COLUMN commitset_commit_owner integer REFERENCES commits DEFAULT NULL, ADD COLUMN commitset_require_build boolean DEFAULT FALSE;'

Please put this in the long description above. You should also describe what they're used for.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:175
> +                array_push($repository_require_build, $repository_id);

This is really a set, right? It's better to use it as an associative array as in:
$repository_require_build[repository_id] = TRUE;
Also, we should rename this to repository_require*s*_build.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:214
> +                $owner_commits = $db->select_first_row('commits', 'commit', array('repository' => intval($repository['repository_owner']), 'revision' => $owner_revision));

This should be $owner_commit, not $owner_commit*s*. You should explicitly check whether we found this commit or not, and throw 'InvalidOwnerRevision' error when it's null.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:216
> +                    exit_with_error('InvalidCommitOwnership', array('commit_owner' => $owner_commits['commit_id'], 'commit_owned' => $commit['commit_id']));

Don't use _ in error messages. Use camelCase instead. But in this case, you don't even need to prefix them. Just call them 'owner' and 'owned'.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:221
> +            array_push($commit_set, array('commit' => $commit['commit_id'], 'patch_file' => $patch_file_id, 'require_build' => $require_build, 'commit_owner' => $owner_commits ? $owner_commits['commit_id'] : NULL));

Instead of adding tertiary, why don't we just declare $owner_commit_id?

> Websites/perf.webkit.org/public/v3/components/analysis-results-viewer.js:129
> -            ]
> +            ];

This change seems unrelated. Can we split that into a separate refactoring patch?

> Websites/perf.webkit.org/public/v3/components/sub-commit-viewer.js:99
> +ComponentBase.defineElement('sub-commit-viewer', SubCommitViewer);

Ditto.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:59
> +    ownedRepositories() { return this._repositories.filter((repository) => { return repository.ownerId(); }) };

Just do: this._repositories.filter((repository) => repository.ownerId);

> Websites/perf.webkit.org/public/v3/models/commit-set.js:212
> +        for (const repository of this._revisionListByOwnedRepository.keys()) {

Do: for (const [repository, thisRevision] of this._revisionListByOwnedRepository) instead.

> Websites/perf.webkit.org/public/v3/models/data-model.js:15
> -            singleton.updateSingleton(object)
> +            singleton.updateSingleton(object);

All these code changes to just add semicolon should really be done separately.

> Websites/perf.webkit.org/public/v3/models/repository.js:32
> +        return ownerships ? ownerships[this.id()].find((repository) => {return repository.name() == name}) : null;

Do: (repository) => repository.name() == name

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:-795
>          });
> -        return this._createTestGroupAfterVerifyingCommitSetList(newName, repetitionCount, commitSetMap);

Please split these cleanups into a separate patch.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:73
> +           assert.deepEqual(content['buildRequests'][0].id, 704);
> +           assert.deepEqual(content['buildRequests'][0].order, 0);
> +           assert.deepEqual(content['buildRequests'][0].platform, '65');
> +           assert.deepEqual(content['buildRequests'][0].commitSet, 403);
> +           assert.deepEqual(content['buildRequests'][0].status, 'pending');
> +           assert.deepEqual(content['buildRequests'][0].test, '200');

Why can't we just do deepEqual(~, {id: 704, order: 0, etc...})?

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:329
> +        let sjc;

nit: jsc

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:333
> +            webkit = Repository.all().filter((repository) => repository.name() == 'WebKit')[0];
> +            macos = Repository.all().filter((repository) => repository.name() == 'macOS')[0];
> +            sjc = Repository.all().filter((repository) => repository.name() == 'JavaScriptCore')[0];

Just declare these variables here.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:613
> +        let sjc;

Nit: jsc

> Websites/perf.webkit.org/tools/js/buildbot-triggerable.js:67
> -            buildReqeustsByGroup = BuildbotTriggerable._testGroupMapForBuildRequests(buildRequests);
> -            return this._pullBuildbotOnAllSyncers(buildReqeustsByGroup);
> +            buildRequestsByGroup = BuildbotTriggerable._testGroupMapForBuildRequests(buildRequests);
> +            return this._pullBuildbotOnAllSyncers(buildRequestsByGroup);

Please do this in a separate cleanup patch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170829/7bf1cb64/attachment.html>