[webkit-reviews] review denied: [Bug 178610] Update perf dashboard upload logic to support uploading binaries from owned commits. : [Attachment 324470] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 20 17:20:45 PDT 2017


Ryosuke Niwa <rniwa at webkit.org> has denied 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 324470: Patch

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




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


More information about the webkit-reviews mailing list