[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