[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