[Webkit-unassigned] [Bug 197973] 'analysis-task-configurator-pane' does not update when switch from one analysis task to another

[bugzilla-daemon at webkit.org]

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #372586|review?                     |review-
              Flags|                            |

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

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

r-. Let's not invent brand new mechanism to write tests just for this one off case.
We can talk about the benefit of improving ways to mock data but we need to tackle that in a separate patch.
In general, it would be a good practice to have a discussion in person or over email before embarking on a big patch like that.

> Websites/perf.webkit.org/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Nit: Need a blank line after this.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:35
> +        const mockData = new ModelV3MockData(context);
> +        await mockData.setup();
> +        mockData.Manifest();

Code like this makes it completely opaque as to what this mock data is setting up.
In general, we really want all the test data to be next to the tests.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:41
> +        // Wait for the RemoteAPI triggered.
> +        await wait(0);

This is precisely what context.symbols.MockRemoteAPI.requests is for. See chart-revision-range-tests.js for an example.

> Websites/perf.webkit.org/browser-tests/mock-data.js:7
> +    'models/data-model.js',

Let's not have yet another list of V3 model files.

> Websites/perf.webkit.org/browser-tests/mock-data.js:60
> +class MockData {

This is too complicated of a setup for mock data.
With this much code running, we have to verify that this mock data setup is correct.
In general, we should keep the test data and its expectation as simple as possible.

> Websites/perf.webkit.org/browser-tests/mock-data.js:226
> +        this._context.iframe.contentWindow.RemoteAPI = {

We don't want to be overriding all tests' RemoteAPI like this.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:165
> +        const fileUploaderEventHandler =  () => {

Nit: Two spaces between = and ().

> Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-form.js:27
> +        if (configurator.hasUserModified()) {
> +            return;
> +        }

Nit: No braces around a single statement.

> Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-form.js:36
> +    resetUserModified() {

Nit: { should appear on the next line.

> Websites/perf.webkit.org/public/v3/components/custom-configuration-test-group-form.js:38
> +        const configurator = this.part('configurator');
> +        configurator.resetUserModified();

This should really be one liner: this.part('configurator').resetUserModified()

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:208
> +        this._currentTestGroups = null;

Why does this need to say "current"? It seems sufficient to say this._testGroup?

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:221
> +        if (!this._currentTestGroups) {

Nit: No braces around a singe line statement.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:224
> +        if (this._currentTestGroups.length !== testGroups.length) {

Nit: Braces.
There is no point in using === here. Just use ==.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:225
> +          return false;

Nit: Wrong indentation.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:228
> +            if (testGroups[i].id() !== this._currentTestGroups[i].id()) {

Again, no need to use !==. id() is always a number.

Nit: Wrong braces & indentation.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:238
> +        if (!this._compareTestGroups(testGroups)) {

Why does it matter that the list of test groups had changed?
This will only happen when the user had unhid hidden test groups or hid a test group.
Why does that matter? Or are you concerned about the case when analysis task had changed?
In that case, a better approach is to pass the analysis task ID as a separate argument, or check the task ID of the test groups.

> Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js:242
> +            if (!this._currentGroup || this._currentGroup.id() !== currentGroup.id()) {

Ditto.

-- 
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/20190625/5ea1b1d0/attachment-0001.html>