[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 #320633|review?                     |review-
              Flags|                            |

--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 320633
  --> https://bugs.webkit.org/attachment.cgi?id=320633
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=320633&action=review

>> Websites/perf.webkit.org/ChangeLog:22
>> +                        ADD CONSTRAINT commitset_item_with_owned_commit_must_requires_build CHECK (commitset_commit_owner IS NULL OR commitset_requires_build = TRUE);
> 
> I may need more comprehensive sql update query which also updates the 'requires_build' field of commit set item, which is from same repository, in the same test group but does not have patch specified, to be true.

As I previously mentioned, please move this to the long description section above this. You shouldn't have something like this as an inline comment.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:98
> +            assert($requires_build !== 't' && $requires_build !== 'f');

I don't think we need to assert this if we're asserting is_bool.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:175
> +        $existing_owner_revisions = array();

I think this should be renamed to something like $owner_revisions_in_set so that it's clear it's existing in the set, not somewhere in the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:216
> +                $repository = $db->select_first_row('repositories', 'repository', array('id' => intval($repository_id)));
> +                if (!$repository)
> +                    exit_with_error('RepositoryNotFound', array('repository' => $repository_id));
> +                $owner_commit = $db->select_first_row('commits', 'commit', array('repository' => intval($repository['repository_owner']), 'revision' => $owner_revision));

There's no need to call intval on these. We've already checked that $repository_id is numeric and repository_owner is a value we're getting out of the database.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:218
> +                    exit_with_error('InvalidOwnerRevision', array('ownerRevision' => $owner_revision));

You should also report the repository. You can just use 'revision', 'repository' as keys since the error name already mentions that they're about an owner revision.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:224
> +            } else {
> +                $existing_owner_revisions[$revision] = TRUE;
> +            }

Nit: a single line statement shouldn't have curly brackets around it:
https://webkit.org/code-style-guidelines/#braces-one-line

This is wrong because multiple repositories can have an identical revision. r- because of this.
I think we should use commit IDs instead.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:227
> +            array_push($commit_set, array('commit' => $commit_id, 'patch_file' => $patch_file_id, 'requires_build' => !!$owner_revision, 'commit_owner' => $owner_commit_id));

We should initialize requires_build to be always FALSE here.
Instead, we should add a code to set repositories_require_build to true when the owner revision is set.

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:230
> +            $repository_commit_set_items_map[$repository_id][] = &$commit_set[count($commit_set) - 1];

This should really be named $repository_to_commit_set_items_map or $commit_set_items_by_repository

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:235
> +            if ($repository && $repository['repository_owner']) {
> +                $repository_owner_list[$repository['repository_owner']] = TRUE;
> +                continue;
> +            }

I don't think we need this code if we checked the commit ID instead.

> Websites/perf.webkit.org/public/v3/models/commit-set.js:178
> +        this._revisionListByOwnedRepository = new Map;

Why don't we just call this _repositoryToOwnerRevisionMap or something similar?

> Websites/perf.webkit.org/public/v3/models/commit-set.js:186
> +        this._revisionListByRepository.set(repository, {revision, patch, ownerRevision});

Here, you have to set _revisionListByOwnedRepository. r- because of this. Please write tests for this.

-- 
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/20170914/dbb1433c/attachment.html>