[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 #22 from Ryosuke Niwa <rniwa at webkit.org> ---
(In reply to Zhifei Fang from comment #21)
> (In reply to Ryosuke Niwa from comment #18)
> > (In reply to Zhifei Fang from comment #17)
> > > (In reply to Ryosuke Niwa from comment #16)
> > > > (In reply to Zhifei Fang from comment #15)
> > > > > I agree to put the mock staff in other patch.
> > > > 
> > > > We should still add tests but with the existing format.
> > > 
> > > I'd love to have something can test the whole page from fetched the network,
> > > this covers all scenario of this patch.
> > 
> > I agree that's valuable but the approach you took isn't right.
> > 
> > > I think the existing format is more easier to be break, we directly create
> > > the data models from constructor or some method like constructXXXFromData,
> > > and need to make sure all the data models have been created in the right
> > > order, mock a fetch, and provide the json directly to the code will be more
> > > easier to maintain.
> > 
> > That's precisely why we don't want the approach you took. We want the test
> > data to be just JSON being sent by the server, and not something from which
> > we construct such a reply. Because then we'd have to be concerned about the
> > correctness of the test code to generate the reply. I have a number of
> > experiences in which such a code was the cause of flaky tests, broken tests,
> > etc...
> > 
> 
> Ok, I can also make a fake server directly, use the existing PHP code to
> server some fake data, we can always fake something directly from server
> side. If we directly use constructors for integration test, it can be easily
> a false pass test. My point is we should cover the whole page logic instead
> of fake data model by data model and put them together. Because we still
> need to worry about the correctness of this part as same as you mentioned.

No. Please don't invent new way to test things in your very first patch to this codebase. We've been writing hundreds of test cases using existing infrastructure so just follow that convention for now. This bug and your patch does not require a brand new infrastructure to write tests.

> > > > > 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.
> > > > 
> > > > Given our existing coding convention is not to use == everywhere, I don't
> > > > think we should do that. Consistency within our own codebase is more
> > > > important than whatever common practice there is elsewhere.
> > > 
> > > I did a search, there are already 35 results in 13 files, and include
> > > something like event.key === "ArrowDown", xxx.length === 1, typeof(value)
> > > === 'number'; Please specify a guide line about when we should use ===, and
> > > when we should use ==. Do we just use if for compare null, undefined, do we
> > > use it when we are sure about the type are the same? I guess the easiest way
> > > and most correct way is we don't use ==, then there are no guide line need
> > > to be followed.
> > 
> > The guideline is not to use === or !== unless it's absolutely necessary. In
> > general, I find that having to treat null, undefined, and 0 to be more of a
> > annoyance. If we're writing code which relies on the difference between
> > them, we have to re-examine the approach in the first place.
> > 
> 
> That's typically not enough, if you really want to default to use == and use
> === when necessary.
> 
> something == false/true 
> // "0" == false => true
> // "1" == false => false
> // "0" == true => false
> // "1" == true => true 
> // etc
> 
> in most case, I'd love to see this has no type conversion

Why are you comparing a string and a boolean in the first place? There should be no issue with that if we're not something that crazy. The preferred way of assert such condition is to write an assertion; e.g. console.log(x instance of Y).

> My point is, if your initial thought about don't type an extra = for saving
> time, you probably will cost more time to think about the type, in most
> case, I cannot see the benefits.

Maybe you do. I don't.

> > > > > This also make the code easier to maintain, think about what if the id() can return something special for special meaning?
> > > > 
> > > > I disagree that always using === would make the code easier to maintain.
> > > > Such an extraordinary claim requires an extraordinary justification, and I
> > > > simply don't see it. And whether other people and/or other projects have
> > > > observed or made such a claim is no basis to adopt the same approach in the
> > > > perf dashboard because each project has its own unique set of requirements
> > > > and challenges as well as a set of people working on it.
> > > 
> > > https://eslint.org/docs/rules/eqeqeq
> > 
> > That's precisely the kind of data I don't care about. I really don't care if
> > someone with Ph.D. in computer sciences or someone coding in JS for 30 years
> > would tell me what. The only thing I care about whether something makes
> > sense for this particular code base, and I don't think it does.
> 
> I think you should treat those case by case, take them all is equal to
> reject them all.

That's right. We need to evaluate the benefit of anything case by case. As for the use of === in the perf dashboard code, I don't see any merit in mandating it given we haven't had an issue with == in the first place.

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