Bug 222463 – Make server test run with new node version

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zhifei Fang]

Make server test run with new node version and exisiting server

[Zhifei Fang]

Created attachment 421623 [details]
Patch

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

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

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

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

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

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.

Working with Zhifei on splitting the node version bump patch out.

[Zhifei Fang]

Created attachment 422307 [details]
Patch

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?

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]

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

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

Created attachment 422319 [details]
Patch

[Zhifei Fang]

Created attachment 422320 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/75084016>

[Zhifei Fang]

Created attachment 422417 [details]
Patch

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]

Created attachment 422430 [details]
Patch

[Zhifei Fang]

Created attachment 422436 [details]
Patch

[Zhifei Fang]

Created attachment 422440 [details]
Patch

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

r=me with comment.

[Ryosuke Niwa]

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

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]

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

I guess an alternative is to use assert.ok(!v) where v can be either undefined or null.

[Ryosuke Niwa]

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

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

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

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

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

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

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

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

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

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

Created attachment 422461 [details]
Patch

[Zhifei Fang]

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

Created attachment 422463 [details]
Patch

[Zhifei Fang]

Created attachment 422465 [details]
Patch

[EWS]

Committed r274036: <https://commits.webkit.org/r274036>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422465 [details].

[Ryosuke Niwa]

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.