<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body><span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span> changed
          <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Performance Dashboard backend should support A/B testing for owned components."
   href="https://bugs.webkit.org/show_bug.cgi?id=175978">bug 175978</a>
          <br>
             <table border="1" cellspacing="0" cellpadding="8">
          <tr>
            <th>What</th>
            <th>Removed</th>
            <th>Added</th>
          </tr>

         <tr>
           <td style="text-align:right;">Attachment #321074 Flags</td>
           <td>review?
           </td>
           <td>review+
           </td>
         </tr></table>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Performance Dashboard backend should support A/B testing for owned components."
   href="https://bugs.webkit.org/show_bug.cgi?id=175978#c11">Comment # 11</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Performance Dashboard backend should support A/B testing for owned components."
   href="https://bugs.webkit.org/show_bug.cgi?id=175978">bug 175978</a>
              from <span class="vcard"><a class="email" href="mailto:rniwa@webkit.org" title="Ryosuke Niwa <rniwa@webkit.org>"> <span class="fn">Ryosuke Niwa</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=321074&action=diff" name="attach_321074" title="Patch">attachment 321074</a> <a href="attachment.cgi?id=321074&action=edit" title="Patch">[details]</a></span>
Patch

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=321074&action=review">https://bugs.webkit.org/attachment.cgi?id=321074&action=review</a>

<span class="quote">> Websites/perf.webkit.org/ChangeLog:12
> +            This will be set true whenever commit_set_item specifies a patch file,</span >

This line shouldn't be indented like this.

<span class="quote">> 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.</span >

Why are these lines further indented?

<span class="quote">> Websites/perf.webkit.org/ChangeLog:18
> +                    UPDATE TABLE commit_set_items SET commitset_requires_build = TRUE WHERE commitset_patch_file IS NOT NULL;</span >

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)

<span class="quote">> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:225
> +            $owner_commit_id = $owner_commit ? $owner_commit['commit_id'] : NULL;</span >

This should be done inside the if statement above since we don't access owner_commit outside the if.

<span class="quote">> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:231
> +            if ($repository && $repository['repository_owner'])</span >

It's cleaner to simply check $owner_commit_id.

<span class="quote">> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:246
> +                exit_with_error('CommitOwnerMustExistInCommitSet', array('owner_commit' => $required_owner_commit));</span >

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.

<span class="quote">> Websites/perf.webkit.org/public/v3/models/repository.js:29
> +    findOwnedRepositoryByName(name)</span >

Looks like this function is never used. Please exclude it from the patch/commit.

<span class="quote">> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:123
>  
> +</span >

Why two blank lines?

<span class="quote">> 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'}}];</span >

Add another variant where the owner repository isn't specified at all.

<span class="quote">> 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', () => {</span >

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.

<span class="quote">> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:54
> +describe('CustomCommitSet', () => {</span >

Nice!</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>