[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 #324470|review?                     |review-
              Flags|                            |

--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 324470
  --> https://bugs.webkit.org/attachment.cgi?id=324470
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.

-- 
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/20171021/7ab9cfe4/attachment.html>