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

Wed Sep 6 00:12:14 PDT 2017

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #319353|review?                     |review-
              Flags|                            |

--- Comment #4 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 319353
  --> https://bugs.webkit.org/attachment.cgi?id=319353
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.

-- 
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/20170906/75705e3b/attachment.html>