Bug 178610 – Update perf dashboard upload logic to support uploading binaries from owned commits.

• Component: New Bugs
• Status: NEW

Update perf dashboard upload logic to support uploading binaries from owned commits.

Created attachment 324470 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 324470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324470&action=review

> Websites/perf.webkit.org/public/api/upload-root.php:69
> +    $commit_repository_rows_in_set = $db->query_and_fetch_all('SELECT commit_set_items.commitset_commit as commitset_commit, owned.repository_name as repository_name, owner.repository_name as owner_repository_name

Please wrap the line after as commitset_commit. It's really hard to read such a long line.

> Websites/perf.webkit.org/public/api/upload-root.php:82
> +            }
> +            else

Nit: else should be on the same line as }.

> Websites/perf.webkit.org/public/api/upload-root.php:91
> +        elseif(is_array($repository_name) && count($repository_name) == 2)

Nit: Use "else if" also need a space between else if and (.
Please use an associative array instead as we discussed in person.

> Websites/perf.webkit.org/public/api/upload-root.php:94
> -            exit_with_error('InvalidRepository', array('repositoryName' => $repository_name, 'commitSet' => $commit_set_id));
> +            exit_with_error('InvalidRepository', array('repositoryName' => $repository_name, 'commitSet' => $commit_set_id, 'full' => $commit_repository_rows_in_set, 'dict' => $commit_by_owned_and_owner_repository_names));

Names like "full" and "dict" are too generic.
I really don't think we should be dumping these entries.
These error messages aren't meant to debug perf dashboard, it's for people writing scripts to submit roots.

> Websites/perf.webkit.org/public/api/upload-root.php:98
> +        exit_with_error('InvalidRepositoryList', array('repositoryList' => $repository_name_list, 'commitSet' => $commit_set_id, 'full' => $commit_repository_rows_in_set, 'dict' => $commit_by_owned_and_owner_repository_names));

Ditto.

> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:625
> -    it('should update all commit set items in the repository listed', () => {
> +    it('should only set build requests as complete when all roots are built', () => {

Instead of modifying the existing test, you should add a new test. r- because of this.

> Websites/perf.webkit.org/server-tests/tools-sync-buildbot-integration-tests.js:809
> +            assert.equal(buildRequest.statusLabel(), 'Running');

Again, we should add a new test case instead of modifying an existing one.

Comment on attachment 324470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324470&action=review

>> Websites/perf.webkit.org/public/api/upload-root.php:94
>> +            exit_with_error('InvalidRepository', array('repositoryName' => $repository_name, 'commitSet' => $commit_set_id, 'full' => $commit_repository_rows_in_set, 'dict' => $commit_by_owned_and_owner_repository_names));
> 
> Names like "full" and "dict" are too generic.
> I really don't think we should be dumping these entries.
> These error messages aren't meant to debug perf dashboard, it's for people writing scripts to submit roots.

You are right. I forgot to remove this debug info.

Comment on attachment 324470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324470&action=review

>> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:625
>> +    it('should only set build requests as complete when all roots are built', () => {
> 
> Instead of modifying the existing test, you should add a new test. r- because of this.

This is a duplicate tests checked in accidentally. It's a dup to the test right above.

Created attachment 324489 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 324489 [details]
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 =.