[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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #338972|review?                     |review+
              Flags|                            |

--- Comment #8 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 338972
  --> https://bugs.webkit.org/attachment.cgi?id=338972
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.

-- 
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/bb617ca8/attachment.html>