Should fetch owner commits in build-requests-fetcher.
Created attachment 332590 [details] Patch
Comment on attachment 332590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332590&action=review > Websites/perf.webkit.org/public/include/build-requests-fetcher.php:135 > + if ($commit_owner_id && !array_key_exists($commit_owner_id, $this->commits_by_id)) > + $commit_owners[$commit_owner_id] = TRUE; Rather than manually finding owner commits like this, we should simply query the database as so: SELECT * FROM commits, repositories WHERE commit_repository = repository_id AND commit_id IN (SELECT commitset_commit FROM commit_set_items WHERE commitset_set = $1);
Created attachment 332607 [details] Patch
Comment on attachment 332607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review > Websites/perf.webkit.org/public/include/build-requests-fetcher.php:149 > + $owner_commits_rows = $this->db->query_and_fetch_all('SELECT * FROM commits WHERE commit_id IN > + (SELECT DISTINCT(commitset_commit_owner) FROM commit_set_items WHERE commitset_set = $1 > + AND commitset_commit_owner IS NOT NULL)', array($commit_set_id)); I would have indented this query as: SELECT * FROM commits WHERE commit_id IN (SELECT DISTINCT(commitset_commit_owner) FROM commit_set_items WHERE commitset_set = $1 AND commitset_commit_owner IS NOT NULL) > Websites/perf.webkit.org/public/include/build-requests-fetcher.php:158 > + 'repository' => $row['commit_repository'], Do: $resolve_ids ? $row['repository_name'] : $row['repository_id']; > Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159 > + 'commitOwner' => NULL, We should assert that $row['commitset_commit_owner'] is NULL. > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:289 > + return MockData.addTestGroupWithOwnerCommitNotInCommitSet(TestServer.database()).then(() => { > + return Manifest.fetch(); > + }).then(() => { > + return BuildRequest.fetchForTriggerable('build-webkit'); > + }).then((buildRequests) => { Use await! await MockData.addTestGroupWithOwnerCommitNotInCommitSet(TestServer.database()); await Manifest.fetch(); const buildRequests = await BuildRequest.fetchForTriggerable('build-webkit'); > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:292 > + let test = Test.findById(200); const. > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:295 > + let platform = Platform.findById(65); const.
Comment on attachment 332607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review >> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:158 >> + 'repository' => $row['commit_repository'], > > Do: $resolve_ids ? $row['repository_name'] : $row['repository_id']; Nice catch. >> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159 >> + 'commitOwner' => NULL, > > We should assert that $row['commitset_commit_owner'] is NULL. $owner_commits_rows only contains information from commits table which does not contains 'commitset_commit_owner'
Comment on attachment 332607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332607&action=review >>> Websites/perf.webkit.org/public/include/build-requests-fetcher.php:159 >>> + 'commitOwner' => NULL, >> >> We should assert that $row['commitset_commit_owner'] is NULL. > > $owner_commits_rows only contains information from commits table which does not contains 'commitset_commit_owner' That just happens to be true for our database today. We always should be asserting those conditions in the case they change. What's the chance you'd remember to fix this code five years from now when that condition changes?