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

[bugzilla-daemon at webkit.org]

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #321074|review?                     |review+
              Flags|                            |

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

-- 
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/20170919/e0531a31/attachment.html>