[Webkit-unassigned] [Bug 178610] Update perf dashboard upload logic to support uploading binaries from owned commits.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=178610

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #324489|review?                     |review+
              Flags|                            |

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

-- 
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/20171023/246736cc/attachment.html>