Bug 182266 – Should fetch owner commits in build-requests-fetcher.

• Component: New Bugs
• Status: NEW

Should fetch owner commits in build-requests-fetcher.

Created attachment 332590 [details]
Patch

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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'

[Ryosuke Niwa]

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?