Bug 168962

Created attachment 302923 [details] Patch

Comment on attachment 302923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302923&action=review > Websites/perf.webkit.org/ChangeLog:9 > + On existing production server, run ''' You should probably use ``` instead. > Websites/perf.webkit.org/ChangeLog:11 > + commit_owner integer NOT NULL REFERENCES commits ON DELETE CASCADE, We should rename repository_parent to repository_owner if we're going with owner/owned. > Websites/perf.webkit.org/public/api/report-commits.php:5 > +function get_repository_id($repository_name, $parent_repoistory_id, $db) { $db must be the first argument. Also { should appear on the following line on its own: https://webkit.org/code-style-guidelines/#braces and no get_ prefix: https://webkit.org/code-style-guidelines/#names-out-argument We should probably call this ensure_repository_id or select_or_insert_repository_row. > Websites/perf.webkit.org/public/api/report-commits.php:10 > + $repository_id = $db->select_or_insert_row('repositories', 'repository', $repository_info); This would happily insert (WebKit, non-null) value yet that would break ReportProcessor::resolve_build_id, ManifestGenerator::repositories (we should include all repositories there with parent repository set), and CommitLogFetcher::repository_id_from_name (should only find top-level repository). > Websites/perf.webkit.org/public/api/report-commits.php:18 > +function process_one_commit($commit_info, $repository_id, $owner_commit_id, $db) { We should probably call insert_commit. Again, $db should be the first argument. > Websites/perf.webkit.org/public/api/report-commits.php:59 > + $db->insert_row('commit_ownerships', '', array('commit_owner' => $owner_commit_id, 'commit_ownee' => $inserted_commit_id), NULL); Just pass in "commit" as the prefix. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:90 > + "WebKit": { We should add a test case where WebKit is a non-sub repository, and make sure submitting it as a sub commit would either fail or insert a new repository. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:92 > + "time": "2013-02-06T08:55:20.9Z", I think we should enforce that sub-commit never specifies commit time because it makes no sense for a sub-commit to have a time and its parent to not have one or them to be different. I think it's best to forbid altogether for simplify for now. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:198 > + const osxCommit = find_first_matching(commits, {'revision': 'Sierra16D32'}); This would not catch this particular revision getting inserted into a wrong repository. It's probably better if we manually fetched repository list, and fetch commit for each instead. Something like this would catch more bugs: (() => { return Promise.all[db.selectAll('repositories'), db.selectAll('commits')]; }).then((result) => { const repositories = result[0]; const repositoryNames = repositories.map((row) => row['repository_name']).sort(); asset.deepEqual(repositoryNames, ["WebKit", ...]); const repositoryIdByName = {}; const commitByRepository = {}; for (let commit of result[1]) { const repsoitoryId = commit['commit_repository']; assert(!(repsoitoryId in commitByRepository)); commitByRepository[repsoitoryId] = commit; } assert(repsoitoryId[repositoryIdByName["WebKit"]]['commit_revision'], '1235'); });

Created attachment 303388 [details] Patch

Comment on attachment 303388 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=303388&action=review > Websites/perf.webkit.org/ChangeLog:18 > + CREATE UNIQUE INDEX repository_name_unique_index ON repositories (repository_name) WHERE repository_owner IS NOT NULL; Please also fix this to say "IS NULL". > Websites/perf.webkit.org/init-database.sql:48 > +WHERE repository_owner IS NOT NULL; Nit: indent WHERE by 4 spaces. > Websites/perf.webkit.org/init-database.sql:50 > +WHERE repository_owner IS NOT NULL; Ditto. This should be WHERE repository_owner IS NULL; > Websites/perf.webkit.org/public/api/report-commits.php:5 > -function main($post_data) { > +function insert_commit($db, $commit_info, $repository_id, $owner_commit_id) Please put this after main before calling main so that they appear in the order of execution. > Websites/perf.webkit.org/public/api/report-commits.php:76 > + $owner_repository_id = $db->select_or_insert_repository_row($commit_info['repository'], NULL); > + if (!$owner_repository_id) { I think we should keep this repository_id. It's confusing to talk about owner_repository_id when we're in the midst of inserting the owner itself. > Websites/perf.webkit.org/public/api/report-commits.php:80 > + $owner_commit_id = insert_commit($db, $commit_info, $owner_repository_id, NULL); Ditto about keeping this just commit_id. > Websites/perf.webkit.org/public/include/db.php:190 > + // FIXME: Should improve _select_update_or_insert_row to handle the NULL column case. Nit: there should be a blank line above this comment. > Websites/perf.webkit.org/public/include/db.php:193 > + $condition = $repository_owner_id != NULL ? '(repository_name, repository_owner) = ($1, $2)' : 'repository_name = $1 AND repository_owner IS NULL'; I think it's cleaner to rewrite this function where we just have two set of queries for when repository_owner_id is null and when it's not. > Websites/perf.webkit.org/public/include/db.php:204 > + > Nit: two blank lines here. > Websites/perf.webkit.org/server-tests/admin-reprocess-report-tests.js:42 > + } Just place this curly bracket at the end of the pervious line. > Websites/perf.webkit.org/server-tests/admin-reprocess-report-tests.js:56 > + assert.equal(repositories.length, 2); You should assert that the only commit that you can get is assigned to the second repository we just inserted. > Websites/perf.webkit.org/server-tests/api-manifest.js:315 > + let webkit1 = Repository.findById(101); Why don't we call this osWebkit or instead of webkit1? > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:81 > + const systemVersionCommitWithSubcommits = { Please move this to where it's used. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:180 > + const sameRepositoryNameInSubCommitAndMajorCommit = { It's better to have this right next to the test. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:419 > + it("should distinguish between repository with same name but different owner", function (done) { Nit: repositories with the asme name but with a different owner. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:429 > + let webkitRepository0 = result[0]; > + let webkitRepository1 = result[1]; I think we should call one of the osWebKit or something. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:433 > + done(); You should verify that each commit is associated with the right repositories. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:444 > + db.selectRows('commits', {'message': 'WebKit Commit'}), We should get the list of all commits for WebKit and verify that we have exactly one commit for WebKit. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:457 > + const osxCommit = result[0][0]; Move each of these right next to where length is asserted so that when one of them fails, it's clear which one it is. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:464 > + assert.notEqual(osxCommit, null); Also move these next to where the variable is declared so that they're next to each other. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:480 > + const ownerCommitForWebKitCommit = result[0][0]; Ditto. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:496 > + db.selectRows('commits', {'message': 'WebKit Commit'}), Please check the number of commits here again. > Websites/perf.webkit.org/server-tests/api-report-commits-tests.js:568 > + assert.equal(commits.length, 1); Please assert that the one commit we have is for the OS.

Created attachment 304239 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 304239 [details]: media/track/track-in-band-style.html bug 153143 (authors: dgorbik@apple.com, eric.carlson@apple.com, and jer.noble@apple.com) The commit-queue is continuing to process your patch.

Comment on attachment 304239 [details] Patch for landing Clearing flags on attachment: 304239 Committed r213788: <http://trac.webkit.org/changeset/213788>

All reviewed patches have been landed. Closing bug.