Bug 184958 – Extend create-analysis-task API to be able to create with confirming test group.

• Component: New Bugs
• Status: RESOLVED FIXED

Extend create-analysis-test api to be able to create with confirming test group.

Created attachment 338713 [details]
Patch

Comment on attachment 338713 [details]
Patch

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

> 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'.

[EWS Watchlist]

Comment on attachment 338713 [details]
Patch

Attachment 338713 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7461112

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 338831 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

Comment on attachment 338713 [details]
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.

[Ryosuke Niwa]

Comment on attachment 338713 [details]
Patch

r- because we probably want to go through another iteration.

Created attachment 338972 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 338972 [details]
Patch

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

> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:77
> +        if (!$triggerable || !$triggerable['id'])
> +            exit_with_error('TriggerableNotFoundForTask', array('task' => $task_id, 'platform' => $config['config_platform']));

Call $db->rollback_transaction() here before exiting.

> Websites/perf.webkit.org/public/privileged-api/create-analysis-task.php:79
> +        if ($triggerable['platform'] != $config['config_platform'])
> +            exit_with_error('InconsistentPlatform', array('configPlatform' => $config['config_platform'], 'taskPlatform' => $triggerable['platform']));

Ditto.

> Websites/perf.webkit.org/public/v3/models/analysis-task.js:310
> +        if (repetitionCount) {
> +            console.assert(testGroupName);

Reverse this. When testGroupName is set, repetitionCount should also be set.
Also give these two arguments default values of null / 0.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:120
> +        const createWithConfirmingTestGroupCheckbox = this.content('create-with-confirming-test-group');

Call this just create-with-test-group.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:238
> +        const name = analyzePopover.querySelector('input[type=text]').value;

Please give it an ID and use this.part(~) to find the element instead.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:239
> +        const createWithConfirmingTestGroup = analyzePopover.querySelector('input[type=checkbox]').checked;

Ditto.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:241
> +        AnalysisTask.create(name, startPoint, endPoint, 'Confirm', createWithConfirmingTestGroup ? repetitionCount : 0).then((data) => {

We shouldn't pass the test group name when we're not creating one.

> Websites/perf.webkit.org/public/v3/pages/chart-pane.js:735
> +            .chart-pane-actions .popover input[type=number] {

In general, we prefer specifying rules using ID than nested descendent selectors like this.

> Websites/perf.webkit.org/server-tests/privileged-api-create-analysis-task-tests.js:497
> +    it('should return "SlaveNotFound" when incorrect slave user and password combination is provided and no analysis task, test group or build request should be created', async () => {

Why don't we add another one which DOES specify slave name & password, and make sure that one works.

rdar://problem/39783538