test, eslintrc: s/assert.equal/assert.strictEqual/ #10698

gibfahn · 2017-01-08T23:33:46Z

Convert assert.equal() to assert.strictEqual() in test/ (no instances found elsewhere by eslint). Also adds a rule to enforce strict equal/notEqual.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test, eslint

gibfahn · 2017-01-08T23:37:07Z

Probably most useful to look at the second and third commits:

2nd (44ee9af) fix issues caused by strict replace.
3rd (f8c6620) add an eslint rule to enforce strictEqual and notStrictEqual

CI: https://ci.nodejs.org/job/node-test-commit/7105/

cc/ @Trott @silverwind @not-an-aardvark

not-an-aardvark · 2017-01-09T00:06:59Z

Rubberstamp LGTM on the first two commits, actual LGTM on the third.

For what it's worth, we could achieve almost the same behavior by configuring the no-restricted-properties rule:

rules:
  no-restricted-properties:
    - error
    - object: assert
      property: equal
      message: use assert.strictEqual() rather than assert.equal()
    - object: assert
      property: notEqual
      message: use assert.notStrictEqual() rather than assert.notEqual()

The only difference is that this custom rule would not warn for something like foo(assert.equal), whereas no-restricted-properties would warn that. I'm not sure whether having a warning would be desirable in that case.

jasnell

largely rubber-stamp LGTM

hiroppy · 2017-01-09T04:56:07Z

wow, awesome!!!

mhdawson

I like it LGTM

gibfahn · 2017-01-10T10:29:02Z

Updated the eslint rule as per @not-an-aardvark's suggestion, if you (or @Trott @targos or @silverwind) could confirm that the changes to .eslintrc are okay that'd be great, especially the changes to __defineGetter__ and __defineSetter__.

Also moved the existing eslint-ignore rule up a bit as otherwise the linter complains about the definitions of assert.equal and assert.notEqual in lib/assert.js.

This should be good to go now

gibfahn · 2017-01-10T10:35:13Z

CI 2: https://ci.nodejs.org/job/node-test-commit/7137/

EDIT: CI is green

targos · 2017-01-10T11:55:10Z

test/parallel/test-process-kill-pid.js

@@ -47,7 +47,7 @@ function kill(tryPid, trySig, expectPid, expectSig) {

  process.kill(tryPid, trySig);

-  assert.strictEqual(getPid, expectPid);
+  assert.strictEqual(getPid.toString(), expectPid.toString());


what are the original types?

This is run several times, and at various times getPid can be a number or a string (expectPid is always a number). I could also do:

assert.strictEqual(Number.parseInt(getPid), expectPid)

if you think that's better.

targos · 2017-01-10T11:57:12Z

.eslintrc

+      message: use assert.strictEqual() rather than assert.equal()
+    - object: assert
+      property: notEqual
+      message: use assert.notStrictEqual() rather than assert.noEqual()


s/noEqual/notEqual/

targos · 2017-01-10T11:58:26Z

.eslintrc

+    - 2
+    - object: assert
+      property: deepEqual
+      message: Please use assert.deepStrictEqual()


Nit add a period at the end and same for the next 2 objects for consistency with the last two (or remove the period for the last two).

not-an-aardvark

LGTM with a nit

not-an-aardvark · 2017-01-10T15:02:43Z

lib/assert.js

@@ -109,7 +109,7 @@ assert.ok = ok;
 // The equality assertion tests shallow, coercive equality with
 // ==.
 // assert.equal(actual, expected, message_opt);
-
+/* eslint-disable no-restricted-properties */


Nit: Maybe // eslint-disable-next-line no-restricted-properties would be better here so that the rule isn't disabled for the rest of the file. (I'm assuming it's intended to only be disabled for this line, rather than for the rest of the file.)

This line was already in use for deepEqual and notDeepEqual below, it's now being used for 4 functions (it's re-enabled below in L136). I think we'd need 8 eslint-disable-next-line lines if we did it individually.

So I left it as it was, let me know if you disagree.

I'd slightly prefer 8 eslint-disable-next-line calls. The more targeted and explicit the disabling, the better. But I don't oppose doing it the way you did it either.

jasnell · 2017-01-10T17:07:08Z

looks like this needs a rebase

gibfahn · 2017-01-10T21:52:22Z

Rebased (twice). If you want to review the equal->strictEqual changes I recommend looking at the squash commit with ?w=1 at the end of the line to ignore whitespace changes.

I mostly fixed issues by adding toString() to things that weren't strings. In some places (e.g. here) it might make more sense to convert things to numbers using Number.parseInt(). I just thought that Number.parseInt() might fail if it gets the wrong input, whereas the toString() should always work (and then you'll get the failure in the assert.strictEqual()). Feedback/disagreement welcome!

jasnell · 2017-01-10T22:00:12Z

Thank you! LGTM

gibfahn · 2017-01-10T22:48:50Z

I'll land this tomorrow if there are no objections (rebasing is painful).

@targos let me know if anything else needs changing.

gibfahn · 2017-01-23T17:23:40Z

@italoacasas Are you saying that this doesn't land cleanly (and thus needs a backport PR) on v7.x?

I don't like the idea of adding the dont-land-on labels before the backport PR has been raised, otherwise things will get forgotten.

EDIT: I tried raising a backport PR, but it looks like the var->const/let changes from #10685 aren't in v7.x-staging yet. Let me know when they are and I'll backport.

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. PR-URL: nodejs#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. PR-URL: nodejs#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. PR-URL: nodejs#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. PR-URL: nodejs#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. PR-URL: nodejs#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

jasnell · 2017-03-08T20:54:51Z

This will need backport PRs in order to land on v6 or v4

bnoordhuis · 2017-03-09T06:59:55Z

This is likely preventing a lot of pull requests from back-porting cleanly so I think it should be back-ported with some urgency. Likewise for #10685.

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. Backport-PR-URL: #11795 PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. Backport-PR-URL: #11795 PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. Backport-PR-URL: #11795 PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. Backport-PR-URL: #11795 PR-URL: #10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Use assert.strictEqual instead of assert.equal in tests, manually convert types where necessary. Backport-PR-URL: nodejs/node#11795 PR-URL: nodejs/node#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

Extend no-restricted-properties to catch use of assert.equal() and assert.notEqual() and require assert.strictEqual() or assert.notStrictEqual() instead. Also update the eslint-ignore in lib/assert.js to avoid assert.equal/notEqual linter errors in their definitions. Backport-PR-URL: nodejs/node#11795 PR-URL: nodejs/node#10698 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Teddy Katz <[email protected]>

nodejs-github-bot added the dont-land-on-v7.x label Jan 8, 2017

mscdex added the test Issues and PRs related to the tests. label Jan 8, 2017

mscdex removed the dont-land-on-v7.x label Jan 9, 2017

jasnell approved these changes Jan 9, 2017

View reviewed changes

hiroppy mentioned this pull request Jan 9, 2017

test: refactor cluster-preload.js #10701

Closed

2 tasks

cjihrig approved these changes Jan 9, 2017

View reviewed changes

mhdawson approved these changes Jan 9, 2017

View reviewed changes

thefourtheye approved these changes Jan 10, 2017

View reviewed changes

gibfahn force-pushed the assert-strict branch from f8c6620 to 156eb32 Compare January 10, 2017 09:44