Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tools,test: enforce deepStrictEqual over deepEqual #6213

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@Trott
Copy link
Member

commented Apr 15, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools, test, benchmark

Description of change

Introduce a lint rule that enforces use of assert.deepStrictEqual()
over assert.deepEqual().

This is a proof of concept inspired by @Fishrock123 wondering in #6196 (comment):

We could make a lint rule that requires strict assertions for the tests, but I feel people may be opposed to that?

I like it, but (like Fishrock123) I don't know if others will feel the same.

If this is deemed a good idea, it can optionally evolve from no-deepEqual to something that flags all non-strict assertions, perhaps only in the tests. Not going to bother, though, if there isn't consensus on this.

@bnoordhuis

View changes

test/parallel/test-buffer-alloc.js Outdated
Buffer.from(expected));
assert.deepEqual(Buffer.from('__--_--_--__', 'base64'),
assert.deepStrictEqual(Buffer.from('__--_--_--__', 'base64'),
Buffer.from(expected));

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 15, 2016

Member

Can you line up the arguments here?

@bnoordhuis

View changes

test/parallel/test-buffer-alloc.js Outdated
@@ -33,7 +33,9 @@ assert.strictEqual(0, d.length);

var ui32 = new Uint32Array(4).fill(42);
var e = Buffer.from(ui32);
assert.deepEqual(ui32, e);
for (var [index, value] of e.entries()) {

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 15, 2016

Member

Is there a reason you use var [k,v] instead of const [k,v] in these loops?

@bnoordhuis

View changes

test/parallel/test-cluster-setup-master-cumulative.js Outdated
@@ -5,11 +5,11 @@ var cluster = require('cluster');

assert(cluster.isMaster);

assert.deepEqual(cluster.settings, {},
assert.deepStrictEqual(cluster.settings, {},
'cluster.settings should not be initialized until needed');

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 15, 2016

Member

Can you line up the arguments?

@bnoordhuis

View changes

test/parallel/test-crypto-binary-default.js Outdated
@@ -363,7 +363,7 @@ assert.equal(a3, '\u00c1(4\u00f1\u0003\u001fd\u0097!O\'\u00d4C/&Qz\u00d4' +
'\u00c2\u0006\u00da0\u00a1\u00879(G\u00ed\'',
'Test SHA512 as assumed binary');

assert.deepEqual(a4,
assert.deepStrictEqual(a4,
Buffer.from('8308651804facb7b9af8ffc53a33a22d6a1c8ac2', 'hex'),

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 15, 2016

Member

Here, too? (And elsewhere? I'll stop pointing it out from now on.)

@bnoordhuis

View changes

test/parallel/test-http-content-length.js Outdated
@@ -10,7 +10,7 @@ var expectedHeadersMultipleWrites = {

var expectedHeadersEndWithData = {
'connection': 'close',
'content-length': 'hello world'.length,
'content-length': '11',

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 15, 2016

Member

String('hello world'.length)?

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

Should we also make assert only importable as assert?

Also, is it possible for the rule to check require('assert').deepEqual()?

Sidenote: while deepEqual() is the most egregious and easiest to mess up, I think we should also discuss not using assert(), assert.equal(), etc. Perhaps in another thread though.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

I can foresee cases where enforcing this rule strictly could cause issues but turning the rule off for individual tests that require lenient handling makes sense... so +1. Haven't reviewed the changes so not giving a LGTM yet.

@Trott Trott force-pushed the Trott:no-deepEqual branch 2 times, most recently Apr 18, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2016

Fixed all of @bnoordhuis's nits, rebased, force pushed.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2016

LGTM

1 similar comment
@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 18, 2016

LGTM

@jasnell

This comment has been minimized.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

@bnoordhuis ... can you take another look?
@Trott ... looks like this needs a rebase

@Trott Trott force-pushed the Trott:no-deepEqual branch Apr 19, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

@jasnell: Rebased and force pushed.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

@Fishrock123 asked:

Should we also make assert only importable as assert?

It's a good question and something I thought about when I wrote the rule.

The answer is yes, but I'd be -0.5 on something like that. There are basically two reasons.

First, it won't stop people from re-assigning:

const assert = require('assert');
const a = assert;
a.deepEqual(foo, bar);

So we're going to have an imperfect rule still. And if we're going to have an imperfect rule, I'd rather have it be simple and imperfect than convoluted and imperfect.

Second, you either have to enforce the identifier name in this rule or you have to create a separate rule to enforce it. If you put it in this rule, it's a bit of scope creep that doesn't quite pass the smell test. This rule is about avoiding deepEqual(). Enforcing that the name of the identifier be assert violates the principle of least astonishment. Ideally, what the rule does should be obvious and unsurprising, at least as much as is feasible. On the other hand, if you make it a separate rule to enforce the identifier name, it's hard to explain why that rule exists. "Oh, it exists because this other rule will miss stuff if the identifier isn't what that rule expects." I don't know, that just doesn't seem to cut it. I'd rather see attempts made to improve the deepEqual() rule so it doesn't need to assume anything about the identifier, although I don't know if that's possible, or at least if it's possible without making the rule very complicated.

Anyway, that was my thinking for not doing that. I'm not 100% opposed, but I am predisposed to be skeptical of going that route. Sorry for the longwinded response to a simple question.
¯_(ツ)_/¯

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

@Fishrock123 asked:

Also, is it possible for the rule to check require('assert').deepEqual()?

Yes. There aren't any instances of that in the current code base. Future enhancement after this bakes in on master for a while (and if it can be done without adding too much complexity to the rule)? Or maybe something to discuss in another thread along with other non-strict assert functions we may want to avoid?

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Another CI, since I rebased and force pushed: https://ci.nodejs.org/job/node-test-pull-request/2310/

EDIT: Well, that certainly didn't go well...

EDIT 2: I'm guessing e38bade is the source of the trouble. We have to modify the tests so that the expected value does not inherit from Object.prototype. This is a good thing as the tests will now make sure that the code in the commit is having the desired effect downstream.

EDIT 3: dba245f too.

@Trott Trott referenced this pull request Apr 19, 2016

Merged

querystring: don't inherit from Object.prototype #6055

3 of 3 tasks complete

@Trott Trott force-pushed the Trott:no-deepEqual branch Apr 19, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

OK, all fixed up. These changes uncovered a bug (or at least I consider it a bug) that is now fixed (or reduced, in any event, if it isn't fixed in all scenarios) in the first commit.

If you looked at it before, it would be great if you could look again because there are some significant changes.

@cjihrig @jasnell @bnoordhuis @mscdex @Fishrock123 @nodejs/testing

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

@Trott ... that fix works for me.

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

CI is good except for one buildbot failure.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

@Trott... Looks like this needs a quick rebase before landing.

test,benchmark: use deepStrictEqual()
In preparation for a lint rule that will enforce
assert.deepStrictEqual() over assert.deepEqual(), change tests and
benchmarks accordingly. For tests and benchmarks that are testing or
benchmarking assert.deepEqual() itself, apply a comment to ignore the
upcoming rule.
tools: enforce deepStrictEqual over deepEqual
Introduce a lint rule that enforces use of `assert.deepStrictEqual()`
over `assert.deepEqual()`.

@Trott Trott force-pushed the Trott:no-deepEqual branch to f0d728c Apr 20, 2016

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

@jasnell Rebased and force-pushed.

Added labels to not land on v4 or v5 because some changes depend on previous semver major changes.

New CI: https://ci.nodejs.org/job/node-test-pull-request/2347/

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2016

Only failure in Ci is a known flaky test.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

LGTM

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Last (or hopefully last) call for additional reviews or objections. If no objections, I'll land this at or soon after 9PM UTC (that's 2PM PDT) tomorrow (Friday).

@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Whoops, meant to do a /cc @nodejs/collaborators on that last comment. Which, for the benefit of those only getting this via email notification, was:

Last (or hopefully last) call for additional reviews or objections. If no objections, I'll land this at or soon after 9PM UTC (that's 2PM PDT) tomorrow (Friday).

Trott added a commit to Trott/io.js that referenced this pull request Apr 22, 2016

test,benchmark: use deepStrictEqual()
In preparation for a lint rule that will enforce
assert.deepStrictEqual() over assert.deepEqual(), change tests and
benchmarks accordingly. For tests and benchmarks that are testing or
benchmarking assert.deepEqual() itself, apply a comment to ignore the
upcoming rule.

PR-URL: nodejs#6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request Apr 22, 2016

tools: enforce deepStrictEqual over deepEqual
Introduce a lint rule that enforces use of `assert.deepStrictEqual()`
over `assert.deepEqual()`.

PR-URL: nodejs#6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

Landed in a7335bd and e84c693

@Trott Trott closed this Apr 22, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

test,benchmark: use deepStrictEqual()
In preparation for a lint rule that will enforce
assert.deepStrictEqual() over assert.deepEqual(), change tests and
benchmarks accordingly. For tests and benchmarks that are testing or
benchmarking assert.deepEqual() itself, apply a comment to ignore the
upcoming rule.

PR-URL: nodejs#6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

tools: enforce deepStrictEqual over deepEqual
Introduce a lint rule that enforces use of `assert.deepStrictEqual()`
over `assert.deepEqual()`.

PR-URL: nodejs#6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

test,benchmark: use deepStrictEqual()
In preparation for a lint rule that will enforce
assert.deepStrictEqual() over assert.deepEqual(), change tests and
benchmarks accordingly. For tests and benchmarks that are testing or
benchmarking assert.deepEqual() itself, apply a comment to ignore the
upcoming rule.

PR-URL: #6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell added a commit that referenced this pull request Apr 26, 2016

tools: enforce deepStrictEqual over deepEqual
Introduce a lint rule that enforces use of `assert.deepStrictEqual()`
over `assert.deepEqual()`.

PR-URL: #6213
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.