Bug 168962 – Add the ability to report a commit with sub-commits.

• Component: Perf Dashboard
• Status: RESOLVED FIXED

Add the ability to report a commit with sub-commits.

Created attachment 302923 [details]
Patch

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 304239 [details]:

media/track/track-in-band-style.html bug 153143 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 304239 [details]
Patch for landing

Clearing flags on attachment: 304239

Committed r213788: <http://trac.webkit.org/changeset/213788>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.