[webkit-reviews] review denied: [Bug 197973] 'analysis-task-configurator-pane' does not update when switch from one analysis task to another : [Attachment 370328] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 197973: 'analysis-task-configurator-pane' does not update when switch from
one analysis task to another
https://bugs.webkit.org/show_bug.cgi?id=197973

Attachment 370328: Patch

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




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

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

Okay, now I understand what you're trying to do but I don't think the fix is
quite right.

> Websites/perf.webkit.org/ChangeLog:9
> +	   Currently on analysis-task-page we check if his children
> +	   custom-configuration-test-group-form already have been set test
groups, if so

I don't really follow what you're trying to say here.

Where / when do we check if custom-configuration-test-group-form already have
test groups set?

> Websites/perf.webkit.org/ChangeLog:11
> +	   we don't update the new test groups
> +	   * browser-tests/analysis-task-page-tests.js: Added a unittest for
analysis task page.

Need a blank line between the long description and file lists.

> Websites/perf.webkit.org/ChangeLog:17
> +	   (CustomAnalysisTaskConfigurator.prototype.setCommitSets): We will
need
> +	   to find the comparisonRepositoryGroup based on the comparison commit
> +	   set, for now it doesn't make any difference since baseline and
> +	   comparison use the same repositoryGroup, however this could be a
potential issue.

Why? What potential issues would that cause?
In general, we strive to add "why" comments instead of "what" comments.
We can see what we're doing from the code,
what would be valuable is explaining why we're doing what we're doing. 
A better thing to say would be something like this:
Fix a bug that we were finding the repository group using the baseline's commit
set instead of comparison's

This should definitely be testable. It should pick a wrong repository group for
the comparison without this fix.

> Websites/perf.webkit.org/ChangeLog:22
> +	   (AnalysisTaskConfiguratorPane.prototype.setTestGroups): When we
first
> +	   go to task A, we will set the form's commit sets, then we redirect
to
> +	   task B, we check if we already set the form's commit set, we already
> +	   have did so, then we don't update the test groups

Okay, this is the kind of description we need below "Reviewed by NOBODY".
We can make it less wordy though.
e.g. The bug was caused by AnalysisTaskConfiguratorPane's setTestGroups
not updating the configuration when the commit sets are already set for another
analysis task.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:190
> +	   const taskConfiguratorPane = new
context.symbols.AnalysisTaskConfiguratorPane;

This test file is called analysis-task-page-tests.js, then create
AnalysisTaskPage.
See browser-tests/test-group-result-page-tests.js for example.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:232
> +	   configurator.render();

This is not correct. You should never differently call "render" method on a
component.
Instead, enqueue the component to render and use waitForComponentsToRender to
wait for it to render.
But really, if we should be enqueuing to render the analysis task page instead.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:235
> +	   expect(configurator.tests()[0]._id).to.be(testGroup1.test()._id);

Don't assert the private states of a class like this. This will make the future
refactoring harder.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:223
> -	   if (!form.hasCommitSets() && currentGroup)
> +	   if (currentGroup)

This isn't right. This fix would re-set test & platform in the configuration
pane whenever a different test group is picked in UI.
It's a feature that the configuration pane retains the manually picked test &
platform even when the user picks another test group.
Please add a test for that.

We probably need to check if !form.hasCommitSets() or analysis task of the
current group has changed.