[webkit-reviews] review granted: [Bug 178610] Update perf dashboard upload logic to support uploading binaries from owned commits. : [Attachment 324489] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 23 16:33:41 PDT 2017
Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 178610: Update perf dashboard upload logic to support uploading binaries
from owned commits.
https://bugs.webkit.org/show_bug.cgi?id=178610
Attachment 324489: Patch
https://bugs.webkit.org/attachment.cgi?id=324489&action=review
--- Comment #6 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 324489
--> https://bugs.webkit.org/attachment.cgi?id=324489
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=324489&action=review
> Websites/perf.webkit.org/public/api/upload-root.php:76
> + $commit_by_owned_and_owner_repository_names = array();
Since this map's owner-major, I think it's better to call it
$commit_by_owner_and_owned_repository_names
> Websites/perf.webkit.org/public/api/upload-root.php:92
> + else if (is_array($repository_name) &&
array_key_exists('ownerRepository', $repository_name) &&
array_key_exists('ownedRepository', $repository_name))
> + $commit_id =
array_get(array_get($commit_by_owned_and_owner_repository_names,
$repository_name['ownerRepository'], array()),
$repository_name['ownedRepository']);
There's no need to check this. You can just array_get ownerRepository and
ownedRepository.
On the other hand, you should spit out an error if either is missing.
> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:13
> +function makeReport(rootFile, buildRequestId = 1, repositoryList=['WebKit'],
buildTime = '2017-05-10T02:54:08.666')
Nit: Need spaces around "=" for repositoryList.
> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:623
> - it('should update all commit set items in the repository listed', () =>
{
> + it('should only set build requests as complete when all roots are
built', () => {
Despite of having the same name, these are two different tests. Please add a
new test case instead.
>
Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:
105
> +function uploadRoot(buildRequestId, buildNumber, repositoryList=["WebKit"],
buildTime='2017-05-10T02:54:08.666')
Nit: Spaces around =.
More information about the webkit-reviews
mailing list