[Webkit-unassigned] [Bug 184958] Extend create-analysis-task API to be able to create with confirming test group.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=184958

--- Comment #5 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 338713
  --> https://bugs.webkit.org/attachment.cgi?id=338713
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338713&action=review

> Websites/perf.webkit.org/ChangeLog:10
> +        Refactored 'create-test-group' API to share some creating test group logic with 'create-analysis-task' API.

Nit: We usually refer to an API by its URL: /privileged-api/create-test-group and /privileged-api/create-analysis-task

> Websites/perf.webkit.org/ChangeLog:11
> +        Moved the shared logic to 'json-header.php'.

Nit: we don't normally quote filenames. Just say json-header.php.

> Websites/perf.webkit.org/ChangeLog:16
> +        Added new helper functions 'create_build_requests_with_configurations_and_order' and

Please use (~) syntax to just functions. Just because prepare-ChangeLogs doesn't generate it,
it doesn't mean we don't need them.

> Websites/perf.webkit.org/ChangeLog:18
> +        'insert_commit_sets_and_construct_configuration_list' which will be shared by both 'create-analysis-task.php' and
> +        'create-test-group.php'.

Ditto about not quoting filenames.

>> Websites/perf.webkit.org/public/include/json-header.php:201
>> +function insert_commit_sets_and_construct_configuration_list($db, $commit_sets)
> 
> How do you like the idea that creating a new file to contain all helper functions those are not related to 'json-header.php'? For example, it seems like 'find_triggerable_for_task' (line169) should not be in 'json-header.php'.

Yes, we should create a new file for this. Maybe commit-sets-helpers.php since all these help functions are commit sets.

> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:11
> +    $confirming_iteration = array_get($data, 'confirmingIteration');

This should be repetitionCount to match privileged-api/create-test-group

> Websites/perf.webkit.org/public/privileged-api/create-test-group.php:96
> +    $order = - count($build_configuration_list);
> +    create_build_requests_with_configurations_and_order($db, $build_configuration_list, $order, $triggerable_id, $platform_id, NULL, $group_id);
> +    assert($order == 0);
> +    for ($i = 0; $i < $repetition_count; $i++)
> +        create_build_requests_with_configurations_and_order($db, $test_configuration_list, $order, $triggerable_id, $platform_id, $test_id, $group_id);

Probably makes sense to extract this entire section as a function.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:306
> -    static create(name, startRunId, endRunId)
> +    static create(name, startPoint, endPoint, testGroupName, confirmingIteration)

If one argument is called testGroupName then the other one shouldn't be called confirmingIteration.
Just call it iterationCount to match the naming convention elsewhere.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:309
> +        if (confirmingIteration > 0) {

Nit: should either check if (testGroupName) or if (iterationCount) instead.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:311
> +            parameters['confirmingIteration'] = confirmingIteration;

This should be called repetitionCount to match /privileged-api/create-test-group.

> Websites/perf.webkit.org/public/v3/models/test-group.js:222
> -    static _revisionSetsFromCommitSets(commitSets)
> +    static revisionSetsFromCommitSets(commitSets)

Maybe this should be a method on CommitSet itself?

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:567
> +                            <label>Confirm with test group of iteration <input type="number" value=4 min=1></label>

This is a long description. How about just "Confirm with X iterations."
We should be using select & option elements here.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:346
> +        let test1 = Test.findByPath(['Suite', 'test1']);
> +        let platform = Platform.findByName('some platform');

Make these const? Also, call it somePlatform to match the name?

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:354
> +        const oneRevisionSet = {};

Do: oneRevisionSet = {[webkitId]: {revision: '191622'}};

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:372
> +    it('should create an analysis task with no test group when confirming repetition is 0', async () => {

Update the test description since we've updated the API to use repetitionCount as well.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:380
> +        let test1 = Test.findByPath(['Suite', 'test1']);
> +        let platform = Platform.findByName('some platform');

Use const here too.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:415
> +    it('should create an analysis task with test group when commit set list and positive repetition count are specified', async () => {

*a* positive repetition count

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:423
> +        let test1 = Test.findByPath(['Suite', 'test1']);
> +        let platform = Platform.findByName('some platform');

Why don't we add these things directly into the database with a known ID?

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:435
> +            db.insert('build_triggerables', {id: 1234, name: 'test-triggerable'}),
> +            db.insert('triggerable_repository_groups', {id: 2345, name: 'webkit-only', triggerable: 1234}),
> +            db.insert('triggerable_repositories', {repository: webkitId, group: 2345}),
> +            db.insert('triggerable_configurations', {test: test1.id(), platform: platform.id(), triggerable: 1234})

Along with these. Then you wouldn't need to fetch Manifest twice.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:448
> +        TestServer.cleanDataDirectory();
> +        await Manifest.fetch();

I'm a worried that clearing all the states like this here may cause us to not catch bugs like the infamous "task is undefined in task.id()" bug we had.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:494
> +describe('/privileged-api/create-analysis-task with node privileged api', function () {

I don't think we need to duplicate all the test cases like this since there isn't any difference in terms of how the server would respond.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:497
> +        PrivilegedAPI.configure('test', 'password');

We need to test that when node node privileged api is used without slave name/password,
the attempt to create an analysis task with/without a test group would fail.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180427/10dca4ef/attachment-0001.html>