[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

--- Comment #15 from Zhifei Fang <zhifei_fang at apple.com> ---
I agree to put the mock staff in other patch.

about === vs ==

My point of view is, the best practice is using === everywhere, since you don't need to think about the type conversion, make sure the type of the value, and always gives you an expected results. And for the same type variables, you have nothing to lose, then why not? This also make the code easier to maintain, think about what if the id() can return something special for special meaning? 

I suggest start to use === everywhere.




(In reply to Ryosuke Niwa from comment #14)
> Comment on attachment 372586 [details]
> 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/e00d07fd/attachment.html>