[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

--- Comment #5 from dewei_zhu at apple.com ---
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/public/include/db.php:122
>> +                $current_value = $this->to_database_boolean($current_value);
> 
> Nice, we should get rid of manual calls to to_database_boolean in a separate patch.

Sure.

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

This seems addressed in previous commit in line 53. I just remove the "FIXME". Should I still mention it in the inline comment?

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

In order to do that, we need search in $commit_set_list to find the right $commit_set, then we also need to search in $commit_set array to find the right commit item. I couldn't find a good way to do that without creating another mapping data structure, which we can avoid if we create $repository_requires_build in advance.

>> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:229
>> +                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.

Sure.

>> 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().

OK, I will include this change in patch for UI change.

>> Websites/perf.webkit.org/public/v3/models/commit-set.js:190
>> +            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.

Do you refer 'two classes' to 'CustomCommitSet' and 'CommitLog'? Could you tell me more about why we want to be consistent here?

>> Websites/perf.webkit.org/public/v3/models/repository.js:34
>> +
> 
> Looks like this is never used for now. Remove?

Sure, it will be used in UI change patch.

>> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:49
>> +           assert.equal(content['commits'][0].id, 87832);
> 
> Why not deepEqual here too?

commits table has a commit_time field which changes between run to run, so we cannot create a commit object that predicts the right time.

-- 
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/20170911/b54a32cc/attachment.html>