[webkit-reviews] review denied: [Bug 175978] Performance Dashboard backend should support A/B testing for owned components. : [Attachment 319353] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Sep 6 00:12:14 PDT 2017


Ryosuke Niwa <rniwa at webkit.org> has denied dewei_zhu at apple.com's request for
review:
Bug 175978: Performance Dashboard backend should support A/B testing for owned
components.
https://bugs.webkit.org/show_bug.cgi?id=175978

Attachment 319353: Patch

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




--- 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.


More information about the webkit-reviews mailing list