Bug 197973 – 'analysis-task-configurator-pane' does not update when switch from one analysis task to another

• Status: NEW
• Component: Tools / Tests

[Zhifei Fang]

<rdar://problem/50741583>

[Zhifei Fang]

Created attachment 370095 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 370095 [details]
Patch

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

> Websites/perf.webkit.org/ChangeLog:13
> +        * public/v3/components/custom-analysis-task-configurator.js:
> +        (CustomAnalysisTaskConfigurator.prototype.setCommitSets):
> +        * public/v3/pages/analysis-task-page.js:
> +        (AnalysisTaskConfiguratorPane.prototype.setTestGroups):

Please explain what you're fixing.
r- due to the lack of explanation about the root cause.

> Websites/perf.webkit.org/ChangeLog:1439
> -        (Statistics.findRangesForChangeDetectionsWithWelchsTTest): The argument `segmentations`, every 2 items in the list defines 
> +        (Statistics.findRangesForChangeDetectionsWithWelchsTTest): The argument `segmentations`, every 2 items in the list defines

Please don't fix past change log entires.

> Websites/perf.webkit.org/ChangeLog:1922
> -        (ReportProcessor::resolve_build_id): 
> +        (ReportProcessor::resolve_build_id):

Ditto.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:8
> +    const fakeTestGroupData1 = {

We don't usually prefix these data entires by fake or mock
since that's pretty self-evident from the fact this is test data.

> Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js:95
> -        const comparisonRepositoryGroup = triggerable.repositoryGroups().find((repositoryGroup) => repositoryGroup.accepts(baselineCommitSet));
> +        const comparisonRepositoryGroup = triggerable.repositoryGroups().find((repositoryGroup) => repositoryGroup.accepts(comparisonCommitSet));

Why are we making this change?

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

Ditto.

[Zhifei Fang]

Created attachment 370325 [details]
Patch

[Zhifei Fang]

Created attachment 370328 [details]
Patch

Comment on attachment 370328 [details]
Patch

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

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:79
> +    },{

Nit, '{' should be a separate line. Ditto for belows.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:174
> +    }

Nit: Add ';' to the end.

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:179
> +        repositoryGroups:[]

Nit, one space after ':'

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:229
> +        }).forEach((buildRequest) => {

For one-liner, we can just do, (buildRequest) => testGroup1.addBuildRequest(buildRequest)

> Websites/perf.webkit.org/browser-tests/analysis-task-page-tests.js:245
> +        });

Ditto

[EWS Watchlist]

Comment on attachment 370328 [details]
Patch

Attachment 370328 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12253214

New failing tests:
css3/filters/blur-various-radii.html

[EWS Watchlist]

Created attachment 370378 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Comment on attachment 370328 [details]
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.

[Zhifei Fang]

Created attachment 371537 [details]
Patch

[EWS Watchlist]

Comment on attachment 371537 [details]
Patch

Attachment 371537 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12402400

New failing tests:
css3/filters/blur-various-radii.html

[EWS Watchlist]

Created attachment 371568 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Zhifei Fang]

Created attachment 372586 [details]
Patch

[Zhifei Fang]

resolved conflict

[Ryosuke Niwa]

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.

[Zhifei Fang]

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.

[Ryosuke Niwa]

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

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

> And for the same type variables, you have nothing to lose, then why not?

Because I'd have to type one more character for no good reason. A dogmatic statement like "we should always do X because that's the best practice" is never helpful without a careful consideration as to whether a given practice is beneficial to the particular project and case we have at hand.

For example, a lot of people who are new to WebKit would claim that we should always wrap each statement in curly braces. Yet, in my 20+ years of writing C/C++ code without braces for single line statements, I've never once had a bug caused by missing curley braces.

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

> I suggest start to use === everywhere.

As such, I'm opposed to this suggestion until such a time is reached that there is a conclusive empirical evidence is provided to support your hypothesis that it materially reduce the maintenance code of the codebase.

[Zhifei Fang]

(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 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. It just mock the backend API and won't explicitly call any data model constructor. So if we modified the data model constructor or constructXXXFromData, the test will still work fine. 

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

> 
> > And for the same type variables, you have nothing to lose, then why not?
> 
> Because I'd have to type one more character for no good reason. A dogmatic
> statement like "we should always do X because that's the best practice" is
> never helpful without a careful consideration as to whether a given practice
> is beneficial to the particular project and case we have at hand.

It will save you thinking time, because each time you use ==, you need to be very sure, that two variables you compare are the same type. 
  
> 
> For example, a lot of people who are new to WebKit would claim that we
> should always wrap each statement in curly braces. Yet, in my 20+ years of
> writing C/C++ code without braces for single line statements, I've never
> once had a bug caused by missing curley braces.

No, this is different than code style, == will have type conversion, === will not, for most people, === produces the expected result. Single line have braces or not doesn't affect the running result.

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

> 
> > I suggest start to use === everywhere.
> 
> As such, I'm opposed to this suggestion until such a time is reached that
> there is a conclusive empirical evidence is provided to support your
> hypothesis that it materially reduce the maintenance code of the codebase.

[Ryosuke Niwa]

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

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

> > > And for the same type variables, you have nothing to lose, then why not?
> > 
> > Because I'd have to type one more character for no good reason. A dogmatic
> > statement like "we should always do X because that's the best practice" is
> > never helpful without a careful consideration as to whether a given practice
> > is beneficial to the particular project and case we have at hand.
> 
> It will save you thinking time, because each time you use ==, you need to be
> very sure, that two variables you compare are the same type.

Well, I've basically written this entire code base from scratch, and I never find that to be case.


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

[Alexey Proskuryakov]

What is the convention for === in Web Inspector? We should certainly consider consistency within the WebKit code base.

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

This is surprising. What about this code base makes it different in this regard?

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #19)
> What is the convention for === in Web Inspector? We should certainly
> consider consistency within the WebKit code base.

https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide doesn't mention this although they seem to mostly use ===. However, I'm not certain that's relevant given there is basically nobody who works on both perf dashboard & web inspector codebase. Quite possibly I'm the only person who ever contributed to both. I do see an argument that using the uniform style across JS code in WebKit is beneficial but that kind of discussion & deployment should happen in a completely different bug than this.

> > 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.
> 
> This is surprising. What about this code base makes it different in this regard?

Because it's a different codebase than anything else. Any style or paradigm of coding should be judged based on its applicability to each codebase.

Consider goto statement for example. Used incorrectly, it could create a bad spaghetti code. Used correctly, it's a very effective control flow statement. Similarly, in WebKit, we use C++ style type cast but there is nothing wrong with C style cast if deployed properly. C++ exceptions are never used in WebKit codebase but it's totally possible to write a good C++ program with them.

In general, I find these kinds of "best practice" preaching to be extremely misguided and harmful. Dogmatic following of such best practices seem to often result in bad code. Take design patterns for example. Many people who deployed design patterns in WebKit before Blink fork ended up making WebKit's codebase a lot worse by introducing many layers of unnecessary abstractions. People who deployed "const" correctness in WebCore's Node code seem to have only increased the code complexity for a little to no benefit as far as I can tell.

[Zhifei Fang]

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

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

something == ''
// 0 == '' => true 

Also, it will support something like
// '1' == 1 => true
to use == in such case encourage people use it to compare things without explicitly convert the type, in my many experience, those can cause bugs very easily.

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.

> > > > And for the same type variables, you have nothing to lose, then why not?
> > > 
> > > Because I'd have to type one more character for no good reason. A dogmatic
> > > statement like "we should always do X because that's the best practice" is
> > > never helpful without a careful consideration as to whether a given practice
> > > is beneficial to the particular project and case we have at hand.
> > 
> > It will save you thinking time, because each time you use ==, you need to be
> > very sure, that two variables you compare are the same type.
> 
> Well, I've basically written this entire code base from scratch, and I never
> find that to be case.
> 
> 
> > > > 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.

[Ryosuke Niwa]

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

[Alexey Proskuryakov]

It looks like the discussion got sidetracked, as the main issue appears to be how tests are written. Is the discussion in the bug still the latest on this topic, or was there an offline discussion already?

Is there anything else that blocks fixing the problem?

I'm hesitantly posting more on the equality operator topic below.

> People who deployed "const" correctness in WebCore's Node code seem to have only increased the code complexity for a little to no benefit as far as I can tell.

This is very different from '==' vs. '==='. The fact that const correctness is broken in C++ is pretty well known (at least now, maybe not as well known when Node const correctness was attempted). The choice of equality operator in JS is something where your position is uncommon, if not unique. There are things about '==' that make reasoning about it harder, and more dependent on overall understanding of the code.

This particular choice should be discussed on webkit-dev in addition to this bug, but the WebKit approach has always been to give people who hack on the code today a stronger say on such matters. Typically, managers/architects/experts only get a chance to convince, not to force.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #23)
> It looks like the discussion got sidetracked, as the main issue appears to
> be how tests are written. Is the discussion in the bug still the latest on
> this topic, or was there an offline discussion already?

Not yet. Yes, we should probably focus on addressing that.

> This is very different from '==' vs. '==='. The fact that const correctness
> is broken in C++ is pretty well known (at least now, maybe not as well known
> when Node const correctness was attempted). The choice of equality operator
> in JS is something where your position is uncommon, if not unique. There are
> things about '==' that make reasoning about it harder, and more dependent on
> overall understanding of the code.
> 
> This particular choice should be discussed on webkit-dev in addition to this
> bug, but the WebKit approach has always been to give people who hack on the
> code today a stronger say on such matters. Typically,
> managers/architects/experts only get a chance to convince, not to force.

I think I tend to agree that I would have used === operator if I wrote perf dashboard code from scratch today. When I initially wrote v3 UI code, === operator was significantly slower than == operator in JSC due to historical reasons. Because I primarily wrote v3 UI code in order to speed up the UI, I opted to mostly use ==.

The problem with start deploying === everywhere is precisely the semantics difference they have with ==. I think we had at least one or two bugs caused by the perf dashboard relying on semantics of == which treats strings and numbers equal whereas Map or Set don't treat them equal in their keys. So in a way, deploying === in the perf dashboard code right now is anything but a major regression risk.

I'm more than open to the discussion of eventually moving to === in the perf dashboard code to be consistent with Web Inspector especially if we end up more contributors who contribute to both but right now, I really don't see an obvious benefit of deploying right away, let alone by someone who had never worked on the perf dashboard code before.

On the contrary, we're very aggressively replacing `var` with `let` and `const` because they can be deployed in each function / block separately, and there is virtually no way of random code elsewhere in the codebase depending on `var` being used in some function. Similarly, we've been deploying class syntax, async, & await because they radically improve the readability of the code by eliminating boilerplates and allowing more natural flow of the code.

So yes, I know the various benefits === provides (and that most JS developers prefer it over ==) but I really don't see much benefit in deploying them in the perf dashboard code today. If anything, there is a serious risk of regression because we literally had them by deploying new data structures that rely on the semantics of ===.