[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