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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 321074: Patch

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




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