WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
222463
Make server test run with new node version
https://bugs.webkit.org/show_bug.cgi?id=222463
Summary
Make server test run with new node version
Zhifei Fang
Reported
2021-02-26 01:02:37 PST
Make server test run with new node version and exisiting server
Attachments
Patch
(881.96 KB, patch)
2021-02-26 01:12 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(856.36 KB, patch)
2021-03-04 16:16 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(855.88 KB, patch)
2021-03-04 18:49 PST
,
Zhifei Fang
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(856.11 KB, patch)
2021-03-04 18:54 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(889.98 KB, patch)
2021-03-05 14:09 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(891.22 KB, patch)
2021-03-05 15:11 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(891.21 KB, patch)
2021-03-05 15:27 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(892.01 KB, patch)
2021-03-05 15:32 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(855.75 KB, patch)
2021-03-05 17:49 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(855.55 KB, patch)
2021-03-05 18:36 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Patch
(855.54 KB, patch)
2021-03-05 19:08 PST
,
Zhifei Fang
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Zhifei Fang
Comment 1
2021-02-26 01:12:20 PST
Created
attachment 421623
[details]
Patch
dewei_zhu
Comment 2
2021-02-26 02:29:46 PST
Comment on
attachment 421623
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=421623&action=review
> Websites/perf.webkit.org/public/v3/models/manifest.js:32 > + static async fetchRawResponse(force=false)
Per convention, we should call it 'no_cache' here.
Ryosuke Niwa
Comment 3
2021-02-27 01:15:19 PST
Comment on
attachment 421623
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=421623&action=review
> Websites/perf.webkit.org/ChangeLog:15 > + (then):
Please remove these bogus inline functions. Clearly there is no function called "then".
> Websites/perf.webkit.org/public/v3/models/manifest.js:25 > - static async fetch() > + static async fetch(force=false)
should be noCache.
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:223 > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}};
Do we really need to parseInt here?
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:325 > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}};
Ditto.
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:362 > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}};
Ditto.
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:400 > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}};
Ditto.
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:436 > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}};
Ditto.
> Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:737 > + assert.deepStrictEqual(commitSet.topLevelRepositories().sort((one, another) => parseInt(one.id()) - parseInt(another.id())), [osx, webkit]);
parseInt here is totally unnecessary.
> Websites/perf.webkit.org/server-tests/api-manifest-tests.js:136 > + assert.strictEqual(webkit.ownedRepositories(), undefined);
Huh, ownedRepositories() should probably be returning null instead in this case. But this is another reason we don't really use strict equality in the codebase. The distinction between undefined & null are decidedly uninteresting.
> Websites/perf.webkit.org/server-tests/api-manifest-tests.js:278 > + assert.strictEqual(someTest.parentTest(), undefined);
Ditto here. parentTest() should be returning null in this case. Looks like this is an issue with DataModel.findById.
> Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:471 > + assert.deepStrictEqual(format(response['formatMap'], currentRows[0]).buildTag, '127'); > + assert.deepStrictEqual(format(response['formatMap'], currentRows[1]).buildTag, '128');
It seems like we can just use assert.strictEqual here? Also, can't we use parseInt like the tests?
> Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:477 > + assert.deepStrictEqual(format(response['formatMap'], baselineRows[0]).buildTag, '131');
Ditto.
> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:858 > + assert.strictEqual(set0.patchForRepository(jsc), undefined); > + assert.strictEqual(set1.patchForRepository(jsc), null);
Weird that we sometimes return undefined and null in other cases. But again, this distinction isn't all that interesting to fix in practice.
> Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1299 > + // pg funtions will return all data as string, so we need make sure that we compare as int
This comment is redundant given we're doing that elsewhere.
> Websites/perf.webkit.org/server-tests/resources/test-server.js:103 > + else
We shouldn't have else per our coding style guideline:
https://webkit.org/code-style-guidelines/#linebreaking-else-if
> Websites/perf.webkit.org/server-tests/resources/test-server.js:153 > + if (this._existingDatabaseConfig) > + return;
Why are we early exiting here? That's precisely why we need to have a noCache option in Manifest.fetch, right? What in new versions of node prevent us from running this code?
> Websites/perf.webkit.org/server-tests/resources/test-server.js:-150 > -
We shouldn't be removing this new line.
> Websites/perf.webkit.org/server-tests/resources/test-server.js:299 > + if (!self._existingDatabaseConfig) { > + self.initDatabase(); > + self.cleanDataDirectory(); > + }
Why can't we re-initialize the database?
> Websites/perf.webkit.org/tools/js/config.js:31 > + all() {
{ should appear on the next line:
https://webkit.org/code-style-guidelines/#braces
Ryosuke Niwa
Comment 4
2021-02-27 01:16:36 PST
(In reply to Zhifei Fang from
comment #0
)
> Make server test run with new node version and exisiting server
Why do we need to make our tests use an existing server? Why is that change desirable?
Zhifei Fang
Comment 5
2021-03-02 14:44:19 PST
(In reply to Ryosuke Niwa from
comment #4
)
> (In reply to Zhifei Fang from
comment #0
) > > Make server test run with new node version and exisiting server > > Why do we need to make our tests use an existing server? Why is that change > desirable?
I am also making a docker compose to avoid developer to install the whole staff. This will make it can run with the docker environment
Zhifei Fang
Comment 6
2021-03-02 14:47:08 PST
(In reply to Ryosuke Niwa from
comment #3
)
> Comment on
attachment 421623
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=421623&action=review
> > > Websites/perf.webkit.org/ChangeLog:15 > > + (then): > > Please remove these bogus inline functions. Clearly there is no function > called "then". > > > Websites/perf.webkit.org/public/v3/models/manifest.js:25 > > - static async fetch() > > + static async fetch(force=false) > > should be noCache. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:223 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Do we really need to parseInt here? > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:325 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:362 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:400 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:436 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:737 > > + assert.deepStrictEqual(commitSet.topLevelRepositories().sort((one, another) => parseInt(one.id()) - parseInt(another.id())), [osx, webkit]); > > parseInt here is totally unnecessary. > > > Websites/perf.webkit.org/server-tests/api-manifest-tests.js:136 > > + assert.strictEqual(webkit.ownedRepositories(), undefined); > > Huh, ownedRepositories() should probably be returning null instead in this > case. > But this is another reason we don't really use strict equality in the > codebase. > The distinction between undefined & null are decidedly uninteresting. > > > Websites/perf.webkit.org/server-tests/api-manifest-tests.js:278 > > + assert.strictEqual(someTest.parentTest(), undefined); > > Ditto here. parentTest() should be returning null in this case. > Looks like this is an issue with DataModel.findById. > > > Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:471 > > + assert.deepStrictEqual(format(response['formatMap'], currentRows[0]).buildTag, '127'); > > + assert.deepStrictEqual(format(response['formatMap'], currentRows[1]).buildTag, '128'); > > It seems like we can just use assert.strictEqual here? > Also, can't we use parseInt like the tests? > > > Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:477 > > + assert.deepStrictEqual(format(response['formatMap'], baselineRows[0]).buildTag, '131'); > > Ditto. > > > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:858 > > + assert.strictEqual(set0.patchForRepository(jsc), undefined); > > + assert.strictEqual(set1.patchForRepository(jsc), null); > > Weird that we sometimes return undefined and null in other cases. > But again, this distinction isn't all that interesting to fix in practice. > > > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1299 > > + // pg funtions will return all data as string, so we need make sure that we compare as int > > This comment is redundant given we're doing that elsewhere. > > > Websites/perf.webkit.org/server-tests/resources/test-server.js:103 > > + else > > We shouldn't have else per our coding style guideline: >
https://webkit.org/code-style-guidelines/#linebreaking-else-if
> > > Websites/perf.webkit.org/server-tests/resources/test-server.js:153 > > + if (this._existingDatabaseConfig) > > + return; > > Why are we early exiting here? > That's precisely why we need to have a noCache option in Manifest.fetch, > right? > What in new versions of node prevent us from running this code? > > > Websites/perf.webkit.org/server-tests/resources/test-server.js:-150 > > - > > We shouldn't be removing this new line. > > > Websites/perf.webkit.org/server-tests/resources/test-server.js:299 > > + if (!self._existingDatabaseConfig) { > > + self.initDatabase(); > > + self.cleanDataDirectory(); > > + } > > Why can't we re-initialize the database? > > > Websites/perf.webkit.org/tools/js/config.js:31 > > + all() { > > { should appear on the next line: >
https://webkit.org/code-style-guidelines/#braces
Some high level commits here: we use pg_* functions, it won't parse the numeric column to int, so a lot of parseInt here is make sure that we are comparing with int but not a string. The new strictEqual is "===", so it's comparing without auto type casting, '1' === 1 will be false, and thus it caused a lot of unit test failed.
Zhifei Fang
Comment 7
2021-03-02 15:53:35 PST
(In reply to Ryosuke Niwa from
comment #3
)
> Comment on
attachment 421623
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=421623&action=review
> > > Websites/perf.webkit.org/ChangeLog:15 > > + (then): > > Please remove these bogus inline functions. Clearly there is no function > called "then". > > > Websites/perf.webkit.org/public/v3/models/manifest.js:25 > > - static async fetch() > > + static async fetch(force=false) > > should be noCache. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:223 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Do we really need to parseInt here? > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:325 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:362 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:400 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:436 > > - status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: content['buildRequests'][0].id}}; > > + status: content['buildRequests'][0].status, url: content['buildRequests'][0].url, buildRequestForRootReuse: parseInt(content['buildRequests'][0].id)}}; > > Ditto. > > > Websites/perf.webkit.org/server-tests/api-build-requests-tests.js:737 > > + assert.deepStrictEqual(commitSet.topLevelRepositories().sort((one, another) => parseInt(one.id()) - parseInt(another.id())), [osx, webkit]); > > parseInt here is totally unnecessary. > > > Websites/perf.webkit.org/server-tests/api-manifest-tests.js:136 > > + assert.strictEqual(webkit.ownedRepositories(), undefined); > > Huh, ownedRepositories() should probably be returning null instead in this > case. > But this is another reason we don't really use strict equality in the > codebase. > The distinction between undefined & null are decidedly uninteresting. > > > Websites/perf.webkit.org/server-tests/api-manifest-tests.js:278 > > + assert.strictEqual(someTest.parentTest(), undefined); > > Ditto here. parentTest() should be returning null in this case. > Looks like this is an issue with DataModel.findById. > > > Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:471 > > + assert.deepStrictEqual(format(response['formatMap'], currentRows[0]).buildTag, '127'); > > + assert.deepStrictEqual(format(response['formatMap'], currentRows[1]).buildTag, '128'); > > It seems like we can just use assert.strictEqual here? > Also, can't we use parseInt like the tests? > > > Websites/perf.webkit.org/server-tests/api-measurement-set-tests.js:477 > > + assert.deepStrictEqual(format(response['formatMap'], baselineRows[0]).buildTag, '131'); > > Ditto. > > > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:858 > > + assert.strictEqual(set0.patchForRepository(jsc), undefined); > > + assert.strictEqual(set1.patchForRepository(jsc), null); > > Weird that we sometimes return undefined and null in other cases. > But again, this distinction isn't all that interesting to fix in practice. > > > Websites/perf.webkit.org/server-tests/privileged-api-create-test-group-tests.js:1299 > > + // pg funtions will return all data as string, so we need make sure that we compare as int > > This comment is redundant given we're doing that elsewhere. > > > Websites/perf.webkit.org/server-tests/resources/test-server.js:103 > > + else > > We shouldn't have else per our coding style guideline: >
https://webkit.org/code-style-guidelines/#linebreaking-else-if
> > > Websites/perf.webkit.org/server-tests/resources/test-server.js:153 > > + if (this._existingDatabaseConfig) > > + return; > > Why are we early exiting here? > That's precisely why we need to have a noCache option in Manifest.fetch, > right?
Yes
> What in new versions of node prevent us from running this code?
Not because of node version, this change is for making the uint tests running with an existing setup, for example an docker env.
> > > Websites/perf.webkit.org/server-tests/resources/test-server.js:-150 > > - > > We shouldn't be removing this new line. > > > Websites/perf.webkit.org/server-tests/resources/test-server.js:299 > > + if (!self._existingDatabaseConfig) { > > + self.initDatabase(); > > + self.cleanDataDirectory(); > > + } > > Why can't we re-initialize the database?
Here is make the server tests run against with existing database service, for example an docker env, so that we cannot easily delete the data folder and recreate it, instead, I change it to truncate all tables and reset all the seqs, along with the no cache for the manifest API.
> > > Websites/perf.webkit.org/tools/js/config.js:31 > > + all() { > > { should appear on the next line: >
https://webkit.org/code-style-guidelines/#braces
Ryosuke Niwa
Comment 8
2021-03-02 18:38:14 PST
Can we split the work to make these tests use Docker into a separate patch? This patch is massive already and it's hard to discuss & review the changes related to Docker from the rest of changes.
dewei_zhu
Comment 9
2021-03-02 22:05:34 PST
Working with Zhifei on splitting the node version bump patch out.
Zhifei Fang
Comment 10
2021-03-04 16:16:32 PST
Created
attachment 422307
[details]
Patch
dewei_zhu
Comment 11
2021-03-04 16:34:24 PST
Comment on
attachment 422307
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422307&action=review
> Websites/perf.webkit.org/ChangeLog:7 > +
You may want to regenerate this commit log since this is a split of the original patch. We no longer change manifest.js in this patch. Also, we made some changes on `markup-component.js` due to console.assert behavior change after node v10.
> Websites/perf.webkit.org/public/v3/models/data-model.js:47 > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null;
How about `return idMap ? idMap[id] || null : null;`
> Websites/perf.webkit.org/public/v3/models/repository.js:52 > + return ownerships ? (ownerships.hasOwnProperty(this.id()) ? ownerships[this.id()] : null) : null;
ditto
> Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:99 > + }).then((result) => { > + return Manifest.fetch().then(() => result);
What is this for?
dewei_zhu
Comment 12
2021-03-04 16:35:37 PST
Updating the patch title to be `Make server test run with new node version` per previous discussion that this one will focus on updating tests.
Zhifei Fang
Comment 13
2021-03-04 16:39:31 PST
(In reply to dewei_zhu from
comment #11
)
> Comment on
attachment 422307
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=422307&action=review
> > > Websites/perf.webkit.org/ChangeLog:7 > > + > > You may want to regenerate this commit log since this is a split of the > original patch. We no longer change manifest.js in this patch. > Also, we made some changes on `markup-component.js` due to console.assert > behavior change after node v10. > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > How about `return idMap ? idMap[id] || null : null;`
incase idMap[id] is 0 / false
> > > Websites/perf.webkit.org/public/v3/models/repository.js:52 > > + return ownerships ? (ownerships.hasOwnProperty(this.id()) ? ownerships[this.id()] : null) : null; > > ditto > > > Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:99 > > + }).then((result) => { > > + return Manifest.fetch().then(() => result); > > What is this for?
Zhifei Fang
Comment 14
2021-03-04 16:43:00 PST
(In reply to dewei_zhu from
comment #11
)
> Comment on
attachment 422307
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=422307&action=review
> > > Websites/perf.webkit.org/ChangeLog:7 > > + > > You may want to regenerate this commit log since this is a split of the > original patch. We no longer change manifest.js in this patch. > Also, we made some changes on `markup-component.js` due to console.assert > behavior change after node v10. > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > How about `return idMap ? idMap[id] || null : null;` > > > Websites/perf.webkit.org/public/v3/models/repository.js:52 > > + return ownerships ? (ownerships.hasOwnProperty(this.id()) ? ownerships[this.id()] : null) : null; > > ditto > > > Websites/perf.webkit.org/server-tests/api-upload-root-tests.js:99 > > + }).then((result) => { > > + return Manifest.fetch().then(() => result); > > What is this for?
If we don't fetch manifest, how will the findById work ?
Zhifei Fang
Comment 15
2021-03-04 18:49:05 PST
Created
attachment 422319
[details]
Patch
Zhifei Fang
Comment 16
2021-03-04 18:54:51 PST
Created
attachment 422320
[details]
Patch
Radar WebKit Bug Importer
Comment 17
2021-03-05 01:03:15 PST
<
rdar://problem/75084016
>
Zhifei Fang
Comment 18
2021-03-05 14:09:31 PST
Created
attachment 422417
[details]
Patch
dewei_zhu
Comment 19
2021-03-05 15:03:03 PST
Comment on
attachment 422417
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422417&action=review
missing console-assert.js
> Websites/perf.webkit.org/ChangeLog:7 > +
Let's give a summary for the change we've done here. Replace assert.equal, assert.deepEqual with assert.strictEqual assert.deepStrictEqual. Replace console.assert under pubic/v3/models with consoleAssert which will throw an exception on assertion failure. Also the return value change when 'undefined' was returned on couple cases.
Zhifei Fang
Comment 20
2021-03-05 15:11:09 PST
Created
attachment 422430
[details]
Patch
Zhifei Fang
Comment 21
2021-03-05 15:27:27 PST
Created
attachment 422436
[details]
Patch
Zhifei Fang
Comment 22
2021-03-05 15:32:39 PST
Created
attachment 422440
[details]
Patch
dewei_zhu
Comment 23
2021-03-05 15:50:11 PST
Comment on
attachment 422440
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> Websites/perf.webkit.org/ChangeLog:9 > + Change assert.equal to assert.strictEqual since node deprecated this API > + Add a new function consoleAssert to make sure it will throw an error > + Make sure model's methods will always return null instead of undefiend > +
This section should below `Reviewed by`, also, sentences will end with '.'.
> Websites/perf.webkit.org/ChangeLog:95 > + (then):
Remove all `(then):` since those are not actually a function. It's a bug in the prepareChangeLog. Same for '(async await):`
> Websites/perf.webkit.org/public/v3/console-assert.js:4 > + //Make sure that browser side we do print out the error
Nit: A space after //.
dewei_zhu
Comment 24
2021-03-05 15:50:41 PST
r=me with comment.
Ryosuke Niwa
Comment 25
2021-03-05 16:03:59 PST
I'm confused here. I thought we decided that we don't want to throw inside the browser? Also, why do we need to add a new function? Why can't server tests simply override the builtin console.assert to make it throw?
Ryosuke Niwa
Comment 26
2021-03-05 16:07:36 PST
Comment on
attachment 422440
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> Websites/perf.webkit.org/public/v3/models/data-model.js:47 > - return idMap ? idMap[id] : null; > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null;
Ugh... let's not do this. It's totally unnecessary to distinguish null and undefined for all intents and purposes.
> Websites/perf.webkit.org/public/v3/models/repository.js:52 > - return ownerships ? ownerships[this.id()] : null; > + return ownerships ? (ownerships.hasOwnProperty(this.id()) ? ownerships[this.id()] : null) : null;
Ditto.
> Websites/perf.webkit.org/public/v3/models/repository.js:63 > - return 0; > + return parseInt(a.id()) - parseInt(b.id());
Again, this parseInt is totally unnecessary. "-" will do the integer conversion automatically.
> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:-26 > - if (deletedAt) { > - console.log(deletedAt);
Let's not modify a random unrelated test.
dewei_zhu
Comment 27
2021-03-05 16:10:26 PST
Comment on
attachment 422440
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
>> Websites/perf.webkit.org/unit-tests/commit-set-tests.js:-26 >> - console.log(deletedAt); > > Let's not modify a random unrelated test.
This is a unnecessary logging checked in from previous change.
Zhifei Fang
Comment 28
2021-03-05 16:12:50 PST
(In reply to Ryosuke Niwa from
comment #26
)
> Comment on
attachment 422440
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > - return idMap ? idMap[id] : null; > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > undefined for all intents and purposes.
I'm confused, if we don't do this, then in the test, since we change to use strictEqual, then we will have different assert against null and undefined, isn't that you don't like? Here is your original comment: " Huh, ownedRepositories() should probably be returning null instead in this case. But this is another reason we don't really use strict equality in the codebase. The distinction between undefined & null are decidedly uninteresting. "
> > > Websites/perf.webkit.org/public/v3/models/repository.js:52 > > - return ownerships ? ownerships[this.id()] : null; > > + return ownerships ? (ownerships.hasOwnProperty(this.id()) ? ownerships[this.id()] : null) : null; > > Ditto. > > > Websites/perf.webkit.org/public/v3/models/repository.js:63 > > - return 0; > > + return parseInt(a.id()) - parseInt(b.id()); > > Again, this parseInt is totally unnecessary. "-" will do the integer > conversion automatically. > > > Websites/perf.webkit.org/unit-tests/commit-set-tests.js:-26 > > - if (deletedAt) { > > - console.log(deletedAt); > > Let's not modify a random unrelated test.
dewei_zhu
Comment 29
2021-03-05 16:15:05 PST
I guess an alternative is to use assert.ok(!v) where v can be either undefined or null.
Ryosuke Niwa
Comment 30
2021-03-05 16:18:18 PST
(In reply to Zhifei Fang from
comment #28
)
> (In reply to Ryosuke Niwa from
comment #26
) > > Comment on
attachment 422440
[details]
> > Patch > > > > View in context: > >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > > - return idMap ? idMap[id] : null; > > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > > undefined for all intents and purposes. > > I'm confused, if we don't do this, then in the test, since we change to use > strictEqual, then we will have different assert against null and undefined, > isn't that you don't like?
Well, the solution to that is don't use === or strict equality for these things. Objects are never equal to one another even with == so there isn't really a point. Anyway, if we really wanted to do this, the right way to write this code would be: return idMap ? (idMap[id] || null) : null; No need to do hasOwnProperty then get_by_id lookup.
Ryosuke Niwa
Comment 31
2021-03-05 16:18:41 PST
(In reply to dewei_zhu from
comment #29
)
> I guess an alternative is to use assert.ok(!v) where v can be either > undefined or null.
Yeah, or assert.ok(v == null).
Zhifei Fang
Comment 32
2021-03-05 16:19:07 PST
(In reply to Ryosuke Niwa from
comment #25
)
> I'm confused here. I thought we decided that we don't want to throw inside > the browser? Also, why do we need to add a new function? Why can't server > tests simply override the builtin console.assert to make it throw?
I think it probably a bad idea that we override the builtin console.assert since we are trying to force node behaves like an old version. How about detect the node env and just throw an error there ?
Ryosuke Niwa
Comment 33
2021-03-05 16:20:12 PST
(In reply to Zhifei Fang from
comment #32
)
> (In reply to Ryosuke Niwa from
comment #25
) > > I'm confused here. I thought we decided that we don't want to throw inside > > the browser? Also, why do we need to add a new function? Why can't server > > tests simply override the builtin console.assert to make it throw? > > I think it probably a bad idea that we override the builtin console.assert > since we are trying to force node behaves like an old version.
Why?
Zhifei Fang
Comment 34
2021-03-05 16:20:50 PST
(In reply to Ryosuke Niwa from
comment #30
)
> (In reply to Zhifei Fang from
comment #28
) > > (In reply to Ryosuke Niwa from
comment #26
) > > > Comment on
attachment 422440
[details]
> > > Patch > > > > > > View in context: > > >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > > > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > > > - return idMap ? idMap[id] : null; > > > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > > > > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > > > undefined for all intents and purposes. > > > > I'm confused, if we don't do this, then in the test, since we change to use > > strictEqual, then we will have different assert against null and undefined, > > isn't that you don't like? > > Well, the solution to that is don't use === or strict equality for these > things. Objects are never equal to one another even with == so there isn't > really a point. > > Anyway, if we really wanted to do this, the right way to write this code > would be: > return idMap ? (idMap[id] || null) : null; > > No need to do hasOwnProperty then get_by_id lookup.
Ok, got it, I do hasOwnProperty just to prevent idMap contains 0 false to prevent us further issue, anyway, will change to assert.ok
Ryosuke Niwa
Comment 35
2021-03-05 16:25:53 PST
(In reply to Zhifei Fang from
comment #34
)
> (In reply to Ryosuke Niwa from
comment #30
) > > (In reply to Zhifei Fang from
comment #28
) > > > (In reply to Ryosuke Niwa from
comment #26
) > > > > Comment on
attachment 422440
[details]
> > > > Patch > > > > > > > > View in context: > > > >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > > > > > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > > > > - return idMap ? idMap[id] : null; > > > > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > > > > > > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > > > > undefined for all intents and purposes. > > > > > > I'm confused, if we don't do this, then in the test, since we change to use > > > strictEqual, then we will have different assert against null and undefined, > > > isn't that you don't like? > > > > Well, the solution to that is don't use === or strict equality for these > > things. Objects are never equal to one another even with == so there isn't > > really a point. > > > > Anyway, if we really wanted to do this, the right way to write this code > > would be: > > return idMap ? (idMap[id] || null) : null; > > > > No need to do hasOwnProperty then get_by_id lookup. > > Ok, got it, I do hasOwnProperty just to prevent idMap contains 0 false to > prevent us further issue, anyway, will change to assert.ok
Yeah but we never those those into idMap. There is also no chance of having prototype property matching id either because id is really PostgreSQL id we receive in most cases or concatenation of those IDs.
Zhifei Fang
Comment 36
2021-03-05 16:31:19 PST
(In reply to Ryosuke Niwa from
comment #33
)
> (In reply to Zhifei Fang from
comment #32
) > > (In reply to Ryosuke Niwa from
comment #25
) > > > I'm confused here. I thought we decided that we don't want to throw inside > > > the browser? Also, why do we need to add a new function? Why can't server > > > tests simply override the builtin console.assert to make it throw? > > > > I think it probably a bad idea that we override the builtin console.assert > > since we are trying to force node behaves like an old version. > > Why?
Well, Node.js has changed this behavior in the new version:
https://nodejs.org/api/console.html#console_console_assert_value_message
vs
https://nodejs.org/dist/latest-v7.x/docs/api/console.html#console_console_assert_value_message_args
Force it back to the old version creates more confusion about node versions. For example, if someone want to add a new test case, they may have some falsy assumption that console.asset won't throw any error (or the other way around), we may need somehow to make sure our behavior is clear.
Zhifei Fang
Comment 37
2021-03-05 16:51:55 PST
(In reply to Ryosuke Niwa from
comment #35
)
> (In reply to Zhifei Fang from
comment #34
) > > (In reply to Ryosuke Niwa from
comment #30
) > > > (In reply to Zhifei Fang from
comment #28
) > > > > (In reply to Ryosuke Niwa from
comment #26
) > > > > > Comment on
attachment 422440
[details]
> > > > > Patch > > > > > > > > > > View in context: > > > > >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > > > > > > > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > > > > > - return idMap ? idMap[id] : null; > > > > > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > > > > > > > > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > > > > > undefined for all intents and purposes. > > > > > > > > I'm confused, if we don't do this, then in the test, since we change to use > > > > strictEqual, then we will have different assert against null and undefined, > > > > isn't that you don't like? > > > > > > Well, the solution to that is don't use === or strict equality for these > > > things. Objects are never equal to one another even with == so there isn't > > > really a point. > > > > > > Anyway, if we really wanted to do this, the right way to write this code > > > would be: > > > return idMap ? (idMap[id] || null) : null; > > > > > > No need to do hasOwnProperty then get_by_id lookup. > > > > Ok, got it, I do hasOwnProperty just to prevent idMap contains 0 false to > > prevent us further issue, anyway, will change to assert.ok > > Yeah but we never those those into idMap. There is also no chance of having > prototype property matching id either because id is really PostgreSQL id we > receive in most cases or concatenation of those IDs.
One more thing, should we consider to modify all the assert.strictEqual(something, null) or we only change the one not working.
Ryosuke Niwa
Comment 38
2021-03-05 17:11:37 PST
(In reply to Zhifei Fang from
comment #37
)
> (In reply to Ryosuke Niwa from
comment #35
) > > (In reply to Zhifei Fang from
comment #34
) > > > (In reply to Ryosuke Niwa from
comment #30
) > > > > (In reply to Zhifei Fang from
comment #28
) > > > > > (In reply to Ryosuke Niwa from
comment #26
) > > > > > > Comment on
attachment 422440
[details]
> > > > > > Patch > > > > > > > > > > > > View in context: > > > > > >
https://bugs.webkit.org/attachment.cgi?id=422440&action=review
> > > > > > > > > > > > > Websites/perf.webkit.org/public/v3/models/data-model.js:47 > > > > > > > - return idMap ? idMap[id] : null; > > > > > > > + return idMap ? (idMap.hasOwnProperty(id) ? idMap[id] : null) : null; > > > > > > > > > > > > Ugh... let's not do this. It's totally unnecessary to distinguish null and > > > > > > undefined for all intents and purposes. > > > > > > > > > > I'm confused, if we don't do this, then in the test, since we change to use > > > > > strictEqual, then we will have different assert against null and undefined, > > > > > isn't that you don't like? > > > > > > > > Well, the solution to that is don't use === or strict equality for these > > > > things. Objects are never equal to one another even with == so there isn't > > > > really a point. > > > > > > > > Anyway, if we really wanted to do this, the right way to write this code > > > > would be: > > > > return idMap ? (idMap[id] || null) : null; > > > > > > > > No need to do hasOwnProperty then get_by_id lookup. > > > > > > Ok, got it, I do hasOwnProperty just to prevent idMap contains 0 false to > > > prevent us further issue, anyway, will change to assert.ok > > > > Yeah but we never those those into idMap. There is also no chance of having > > prototype property matching id either because id is really PostgreSQL id we > > receive in most cases or concatenation of those IDs. > > One more thing, should we consider to modify all the > assert.strictEqual(something, null) or we only change the one not working.
Just the ones that broke I guess.
Ryosuke Niwa
Comment 39
2021-03-05 17:20:09 PST
(In reply to Zhifei Fang from
comment #36
)
> (In reply to Ryosuke Niwa from
comment #33
) > > (In reply to Zhifei Fang from
comment #32
) > > > (In reply to Ryosuke Niwa from
comment #25
) > > > > I'm confused here. I thought we decided that we don't want to throw inside > > > > the browser? Also, why do we need to add a new function? Why can't server > > > > tests simply override the builtin console.assert to make it throw? > > > > > > I think it probably a bad idea that we override the builtin console.assert > > > since we are trying to force node behaves like an old version. > > > > Why? > > Well, Node.js has changed this behavior in the new version: > >
https://nodejs.org/api/console.html#console_console_assert_value_message
> > vs > >
https://nodejs.org/dist/latest-v7.x/docs/api/console
. > html#console_console_assert_value_message_args > > Force it back to the old version creates more confusion about node versions.
Why would it? I don't think anyone will guess which node version we're using based on whether console.assert throws or not. If anything, it's a benefit that console.assert will the same thing in old & new versions of node.js so that we don't have to worry about the version difference between the two. We don't really want to have some implicit rule about when & when not to use console.assert in the JS code shared between browser UI & backend JS script.
> For example, if someone want to add a new test case, they may have some > falsy assumption that console.asse[r]t won't throw any error (or the other way > around), we may need somehow to make sure our behavior is clear.
It is a fair point that this behavior might be surprising to someone who is new to this codebase but we don't really have anyone else contributing to this codebase so I'm not sure if that's an important consideration. As a general principle, it's probably a good idea to reduce these kinds of overrides on the functionality of built-in features but I'm more concerned about making sure our codebase survives this node.js transition smoothly than introducing that kind of mild theoretical inconsistency with that's in the ECMA spec vs. what we do. Also, what are scenarios in which this confusion about whether console.assert will or will not result in an issue? If someone is writing a new test, I hope that person is checking whether the test case will fail as expected in the case there was a bug the test is intending to test. Given we're making console.assert throw, there is no issuing writing a test case that accidentally throws because that should be caught by the person writing the test. If the person had assumed that console.assert doesn't throw but added console.assert anyway, then the person should notice that it indeed throws when running tests. In either scenario, I don't see this behavior difference resulting in any bad consequences.
Zhifei Fang
Comment 40
2021-03-05 17:49:28 PST
Created
attachment 422461
[details]
Patch
Zhifei Fang
Comment 41
2021-03-05 17:58:57 PST
Changed to override (In reply to Ryosuke Niwa from
comment #39
)
> (In reply to Zhifei Fang from
comment #36
) > > (In reply to Ryosuke Niwa from
comment #33
) > > > (In reply to Zhifei Fang from
comment #32
) > > > > (In reply to Ryosuke Niwa from
comment #25
) > > > > > I'm confused here. I thought we decided that we don't want to throw inside > > > > > the browser? Also, why do we need to add a new function? Why can't server > > > > > tests simply override the builtin console.assert to make it throw? > > > > > > > > I think it probably a bad idea that we override the builtin console.assert > > > > since we are trying to force node behaves like an old version. > > > > > > Why? > > > > Well, Node.js has changed this behavior in the new version: > > > >
https://nodejs.org/api/console.html#console_console_assert_value_message
> > > > vs > > > >
https://nodejs.org/dist/latest-v7.x/docs/api/console
. > > html#console_console_assert_value_message_args > > > > Force it back to the old version creates more confusion about node versions. > > Why would it? I don't think anyone will guess which node version we're using > based on whether console.assert throws or not. > > If anything, it's a benefit that console.assert will the same thing in old & > new versions of node.js so that we don't have to worry about the version > difference between the two. We don't really want to have some implicit rule > about when & when not to use console.assert in the JS code shared between > browser UI & backend JS script. > > > For example, if someone want to add a new test case, they may have some > > falsy assumption that console.asse[r]t won't throw any error (or the other way > > around), we may need somehow to make sure our behavior is clear. > > It is a fair point that this behavior might be surprising to someone who is > new to this codebase but we don't really have anyone else contributing to > this codebase so I'm not sure if that's an important consideration. As a > general principle, it's probably a good idea to reduce these kinds of > overrides on the functionality of built-in features but I'm more concerned > about making sure our codebase survives this node.js transition smoothly > than introducing that kind of mild theoretical inconsistency with that's in > the ECMA spec vs. what we do. > > Also, what are scenarios in which this confusion about whether > console.assert will or will not result in an issue? If someone is writing a > new test, I hope that person is checking whether the test case will fail as > expected in the case there was a bug the test is intending to test. Given > we're making console.assert throw, there is no issuing writing a test case > that accidentally throws because that should be caught by the person writing > the test. If the person had assumed that console.assert doesn't throw but > added console.assert anyway, then the person should notice that it indeed > throws when running tests. In either scenario, I don't see this behavior > difference resulting in any bad consequences.
Ryosuke Niwa
Comment 42
2021-03-05 18:05:24 PST
Comment on
attachment 422461
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=422461&action=review
Nice! This is a lot cleaner approach.
> Websites/perf.webkit.org/tools/js/assert-override.js:3 > +function makeConsoleAssertThrow () {
Nit: no space between the function name and (). Also { should appear on the new line.
Zhifei Fang
Comment 43
2021-03-05 18:36:04 PST
Created
attachment 422463
[details]
Patch
Zhifei Fang
Comment 44
2021-03-05 19:08:00 PST
Created
attachment 422465
[details]
Patch
EWS
Comment 45
2021-03-06 06:32:01 PST
Committed
r274036
: <
https://commits.webkit.org/r274036
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 422465
[details]
.
Ryosuke Niwa
Comment 46
2021-03-06 18:10:43 PST
Oops, turns out that we have a bunch of browser tests for markup-component.js so adding require call there broke those tests. Fixing this in the
bug 222874
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug