[webkit-reviews] review granted: [Bug 184958] Extend create-analysis-task API to be able to create with confirming test group. : [Attachment 338972] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted dewei_zhu at apple.com's request for
review:
Bug 184958: Extend create-analysis-task API to be able to create with
confirming test group.
https://bugs.webkit.org/show_bug.cgi?id=184958

Attachment 338972: Patch

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




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