Summary: | Update perf dashboard upload logic to support uploading binaries from owned commits. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | dewei_zhu | ||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | NEW --- | ||||||||
Severity: | Normal | CC: | dewei_zhu, rniwa | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
dewei_zhu
2017-10-20 16:56:02 PDT
Created attachment 324470 [details]
Patch
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
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 =. |