Bug 175978 – Performance Dashboard backend should support A/B testing for owned components.

• Status: NEW
• Component: New Bugs

Performance Dashboard should support A/B testing for owned components.

Created attachment 319071 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 319071 [details]
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.

Created attachment 319353 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 319353 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:9
> +        * init-database.sql:

You should have a blank line right above this.

> Websites/perf.webkit.org/ChangeLog:10
> +            Added 'commitset_commit_owner' and 'commitset_requires_build' columns to 'commit_set_items' table.

Please add this right after the description above. These important high-level comments should not be added as an inline comment.

> Websites/perf.webkit.org/ChangeLog:24
> +        * public/privileged-api/create-test-group.php: Added logic to process 'commitOwner' and 'requireBuild' in 'commit_set_items'.

Please add entries for each function you're modifying with inline comments.

> Websites/perf.webkit.org/ChangeLog:49
> +        (return.MockData.addTestGroupWithOwnedRepositories.TestServer.database.then):
> +        (then):

Get rid of these entries.

> Websites/perf.webkit.org/ChangeLog:52
> +        (return.addTriggerableAndCreateTask.string_appeared_here.then.id.taskId.id.then):
> +        (then):

Ditto.

> Websites/perf.webkit.org/public/include/db.php:122
> +            if (is_bool($current_value))
> +                $current_value = $this->to_database_boolean($current_value);

Nice, we should get rid of manual calls to to_database_boolean in a separate patch.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:-55
> -        // FIXME: Add a check for duplicate test group name.

Mention that this FIXME is already addressed in the change log as an inline comment.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:96
> +            $need_to_build = $need_to_build || $commit_row['requires_build'];

We should add an assert here that is_bool($commit_row['requires_build']) is true, and it's never 't' or 'f'.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:207
> +            $repository = $db->select_first_row('repositories', 'repository', array('id' => intval($repository_id)));

I don't think we need to fetch the repository row when it doesn't have an owner revision.
So we should move this into if ($owner_revision).

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:221
> +            $requires_build = $owner_revision || array_get($repository_requires_build, $repository_id, FALSE);

I think it's cleaner to update $repository_requires_build here and add another loop after (instead of before) this one,
and update values of requires_build in each commit_set object in the newly added loop once all commit_set objects are created.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:227
> +            if (!$repository)
> +                exit_with_error('RepositoryNotFound', array('repository' => $repository_id));

This condition is impossible to reach because we've already used $repository_id to find $commit_id.
Also, if this condition can be reached, then we need to be checking this condition before line 215 where we use it.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:229
> +            if ($repository['repository_owner'])
> +                continue;

We need to verify that its owner is in the list. It's non-sensical to have a owned repository specified without its owner's revision being specified.
r- due to the lack of this check. We also need a test case for this.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:13
> +        this._repositoryToRequiresBuildMap = new Map;

Nit: It should be _repositoryRequiresBuildMap. Can't be "to requires".

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

Looks like this function is never used. Remove for now?
Also, I would have called this repositoriesWithOwner().

> Websites/perf.webkit.org/public/v3/models/commit-set.js:190
> -        this._revisionListByRepository.set(repository, {revision, patch});
> +        if (ownerRevision)
> +            this._revisionListByOwnedRepository.set(repository, {revision, patch, ownerRevision});
> +        else
> +            this._revisionListByRepository.set(repository, {revision, patch});

We should stick with either putting all revisions into a single map, and have a separate owner map as done in a regular CommitLog,
or have two separate maps for owned revisions and non-owned revisions as done here.
We shouldn't use two completely different strategies for the two classes.

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

Looks like this is never used for now. Remove?

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:30
> +    it('should return build requests associated with a given triggerable with appropriate commits and commitSets with owned components.', () => {

Nit: owned commits. Also, don't end it with a period.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:41
> +               [{commit: '87832', commitOwner: null, patch: null, requiresBuild: false, rootFile: null},
> +                   {commit: '93116', commitOwner: null, patch: null, requiresBuild: false, rootFile: null},
> +                   {commit: '1797', commitOwner: "93116", patch: null, requiresBuild: true, rootFile: null}]);

This indentation is not right. You should start [ in the previous line, and then all { should be aligned.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:44
> +               [{commit: '87832', commitOwner: null, patch: null, requiresBuild: false, rootFile: null},

Ditto.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:49
> +           assert.equal(content['commits'][0].id, 87832);

Why not deepEqual here too?

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:68
> +           assert.deepEqual(content['buildRequests'][0].id, 704);

Ditto.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:152
>  
> +

Why two spaces?

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:288
> +            assert.equal(secondCommitSet.revisionForRepository(jsc), '9191');

You should check that the owner of jsc is osx and that ownerRevisionForRepository of this commit is that of osx.
r- due to this lack of test coverage.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:379
> +

Add another test case which specifies an owned revision but not its owner revision.

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

We should add LabeledObject.findByName at some point...

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:753
> +            assert.equal(set0.patchForRepository(jsc), null);
> +            assert.equal(set1.patchForRepository(jsc), null);

Again, you should test the owner commit, repository, etc...

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:836
> +            assert.equal(set1.requiresBuildForRepository(jsc), true);

Ditto.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:37
> +            db.insert('commits', {id: 1797, repository: this.jscRepositoryId(), revision: '6161', time: '2016-03-02T23:19:55.3Z'}),
> +            db.insert('commits', {id: 2017, repository: this.jscRepositoryId(), revision: '9191', time: '2016-05-02T23:13:57.1Z'}),

Can we use a human readable text like '191622-jsc' instead of these unintelligible numbers?
Also, we definitely need a test case for when both OS X / iOS and WebKit specifies WebKit/JavaScriptCore.

> Websites/perf.webkit.org/server-tests/resources/mock-data.js:123
> +    addTestGroupWithOwnedRepositories(db, statusList)

I'd call this addTestGroupWithOwnedCommits instead.

Comment on attachment 319353 [details]
Patch

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

>> Websites/perf.webkit.org/public/include/db.php:122
>> +                $current_value = $this->to_database_boolean($current_value);
> 
> Nice, we should get rid of manual calls to to_database_boolean in a separate patch.

Sure.

>> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:-55
>> -        // FIXME: Add a check for duplicate test group name.
> 
> Mention that this FIXME is already addressed in the change log as an inline comment.

This seems addressed in previous commit in line 53. I just remove the "FIXME". Should I still mention it in the inline comment?

>> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:221
>> +            $requires_build = $owner_revision || array_get($repository_requires_build, $repository_id, FALSE);
> 
> I think it's cleaner to update $repository_requires_build here and add another loop after (instead of before) this one,
> and update values of requires_build in each commit_set object in the newly added loop once all commit_set objects are created.

In order to do that, we need search in $commit_set_list to find the right $commit_set, then we also need to search in $commit_set array to find the right commit item. I couldn't find a good way to do that without creating another mapping data structure, which we can avoid if we create $repository_requires_build in advance.

>> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:229
>> +                continue;
> 
> We need to verify that its owner is in the list. It's non-sensical to have a owned repository specified without its owner's revision being specified.
> r- due to the lack of this check. We also need a test case for this.

Sure.

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:59
>> +    ownedRepositories() { return this._repositories.filter((repository) => repository.ownerId) };
> 
> Looks like this function is never used. Remove for now?
> Also, I would have called this repositoriesWithOwner().

OK, I will include this change in patch for UI change.

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:190
>> +            this._revisionListByRepository.set(repository, {revision, patch});
> 
> We should stick with either putting all revisions into a single map, and have a separate owner map as done in a regular CommitLog,
> or have two separate maps for owned revisions and non-owned revisions as done here.
> We shouldn't use two completely different strategies for the two classes.

Do you refer 'two classes' to 'CustomCommitSet' and 'CommitLog'? Could you tell me more about why we want to be consistent here?

>> Websites/perf.webkit.org/public/v3/models/repository.js:34
>> +
> 
> Looks like this is never used for now. Remove?

Sure, it will be used in UI change patch.

>> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:49
>> +           assert.equal(content['commits'][0].id, 87832);
> 
> Why not deepEqual here too?

commits table has a commit_time field which changes between run to run, so we cannot create a commit object that predicts the right time.

Created attachment 320633 [details]
Patch

Comment on attachment 320633 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:22
> +                    ALTER TABLE commit_set_items ADD COLUMN commitset_commit_owner integer REFERENCES commits DEFAULT NULL, ADD COLUMN commitset_requires_build boolean DEFAULT FALSE;
> +                    UPDATE TABLE commit_set_items SET commitset_requires_build = TRUE WHERE commitset_patch_file IS NOT NULL;
> +                    ALTER TABLE commit_set_items ADD commitset_item_with_patch_must_requires_build CHECK (commitset_patch_file IS NULL OR commitset_requires_build = TRUE),
> +                        ADD CONSTRAINT commitset_item_with_owned_commit_must_requires_build CHECK (commitset_commit_owner IS NULL OR commitset_requires_build = TRUE);

I may need more comprehensive sql update query which also updates the 'requires_build' field of commit set item, which is from same repository, in the same test group but does not have patch specified, to be true.

[Ryosuke Niwa]

Comment on attachment 320633 [details]
Patch

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

>> Websites/perf.webkit.org/ChangeLog:22
>> +                        ADD CONSTRAINT commitset_item_with_owned_commit_must_requires_build CHECK (commitset_commit_owner IS NULL OR commitset_requires_build = TRUE);
> 
> I may need more comprehensive sql update query which also updates the 'requires_build' field of commit set item, which is from same repository, in the same test group but does not have patch specified, to be true.

As I previously mentioned, please move this to the long description section above this. You shouldn't have something like this as an inline comment.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:98
> +            assert($requires_build !== 't' && $requires_build !== 'f');

I don't think we need to assert this if we're asserting is_bool.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:175
> +        $existing_owner_revisions = array();

I think this should be renamed to something like $owner_revisions_in_set so that it's clear it's existing in the set, not somewhere in the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:216
> +                $repository = $db->select_first_row('repositories', 'repository', array('id' => intval($repository_id)));
> +                if (!$repository)
> +                    exit_with_error('RepositoryNotFound', array('repository' => $repository_id));
> +                $owner_commit = $db->select_first_row('commits', 'commit', array('repository' => intval($repository['repository_owner']), 'revision' => $owner_revision));

There's no need to call intval on these. We've already checked that $repository_id is numeric and repository_owner is a value we're getting out of the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:218
> +                    exit_with_error('InvalidOwnerRevision', array('ownerRevision' => $owner_revision));

You should also report the repository. You can just use 'revision', 'repository' as keys since the error name already mentions that they're about an owner revision.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:224
> +            } else {
> +                $existing_owner_revisions[$revision] = TRUE;
> +            }

Nit: a single line statement shouldn't have curly brackets around it:
https://webkit.org/code-style-guidelines/#braces-one-line

This is wrong because multiple repositories can have an identical revision. r- because of this.
I think we should use commit IDs instead.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:227
> +            array_push($commit_set, array('commit' => $commit_id, 'patch_file' => $patch_file_id, 'requires_build' => !!$owner_revision, 'commit_owner' => $owner_commit_id));

We should initialize requires_build to be always FALSE here.
Instead, we should add a code to set repositories_require_build to true when the owner revision is set.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:230
> +            $repository_commit_set_items_map[$repository_id][] = &$commit_set[count($commit_set) - 1];

This should really be named $repository_to_commit_set_items_map or $commit_set_items_by_repository

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:235
> +            if ($repository && $repository['repository_owner']) {
> +                $repository_owner_list[$repository['repository_owner']] = TRUE;
> +                continue;
> +            }

I don't think we need this code if we checked the commit ID instead.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:178
> +        this._revisionListByOwnedRepository = new Map;

Why don't we just call this _repositoryToOwnerRevisionMap or something similar?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:186
> +        this._revisionListByRepository.set(repository, {revision, patch, ownerRevision});

Here, you have to set _revisionListByOwnedRepository. r- because of this. Please write tests for this.

Created attachment 321067 [details]
Patch

Created attachment 321074 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 321074 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:12
> +            This will be set true whenever commit_set_item specifies a patch file,

This line shouldn't be indented like this.

> Websites/perf.webkit.org/ChangeLog:14
> +                or commit_set_item is commit with owner commit,
> +                or any other commit from same repository and in same build-request group requires build.

Why are these lines further indented?

> Websites/perf.webkit.org/ChangeLog:18
> +                    UPDATE TABLE commit_set_items SET commitset_requires_build = TRUE WHERE commitset_patch_file IS NOT NULL;

You should also add a query to update every commit_set_items in the same test group.
Something like this (please test it locally):
UPDATE TABLE commit_set_items SET commitset_requires_build = TRUE WHERE commitset_set IN
(SELECT requests1.request_commit_set FROM build_requests as requests1 JOIN build_requests as requests2
ON requests1.request_group = requests2.request_group WHERE requests2.commitset_patch_file IS NOT NULL)

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:225
> +            $owner_commit_id = $owner_commit ? $owner_commit['commit_id'] : NULL;

This should be done inside the if statement above since we don't access owner_commit outside the if.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:231
> +            if ($repository && $repository['repository_owner'])

It's cleaner to simply check $owner_commit_id.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:246
> +                exit_with_error('CommitOwnerMustExistInCommitSet', array('owner_commit' => $required_owner_commit));

I think it would be useful to spit out the owned commit as well. Just store the owned commit ID as the value instead of TRUE in required_owner_commits.

> Websites/perf.webkit.org/public/v3/models/repository.js:29
> +    findOwnedRepositoryByName(name)

Looks like this function is never used. Please exclude it from the patch/commit.

> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:123
>  
> +

Why two blank lines?

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:392
> +            const revisionSets = [{[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-6161', ownerRevision: '191622'}},
> +                {[webkit.id()]: {revision: '191622'}, [macos.id()]: {revision: '15A284'}, [jsc.id()]: {revision: 'owned-jsc-9191', ownerRevision: '192736'}}];

Add another variant where the owner repository isn't specified at all.

> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:782
> +    it('should create a test group with a owned commits and a patch', () => {

Add another variant where one of the revision doesn't specify owned commit. e.g. one set specifies system JSC and the other one doesn't.
Make sure that requires_build is specified for both sets.

> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:54
> +describe('CustomCommitSet', () => {

Nice!