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

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

Merged
merged 2 commits into from
Jan 11, 2017

Conversation

gibfahn
Copy link
Member

@gibfahn gibfahn commented Jan 8, 2017

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
Copy link
Member Author

gibfahn commented Jan 8, 2017

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

@mscdex mscdex added the test Issues and PRs related to the tests. label Jan 8, 2017
@not-an-aardvark
Copy link
Contributor

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.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

largely rubber-stamp LGTM

@hiroppy
Copy link
Member

hiroppy commented Jan 9, 2017

wow, awesome!!!

@hiroppy hiroppy mentioned this pull request Jan 9, 2017
2 tasks
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it LGTM

@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

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
Copy link
Member Author

gibfahn commented Jan 10, 2017

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

EDIT: CI is green

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

process.kill(tryPid, trySig);

assert.strictEqual(getPid, expectPid);
assert.strictEqual(getPid.toString(), expectPid.toString());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the original types?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/noEqual/notEqual/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

- 2
- object: assert
property: deepEqual
message: Please use assert.deepStrictEqual()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit

@@ -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 */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@gibfahn gibfahn Jan 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jasnell commented Jan 10, 2017

looks like this needs a rebase

@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

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
Copy link
Member

jasnell commented Jan 10, 2017

Thank you! LGTM

@gibfahn
Copy link
Member Author

gibfahn commented Jan 10, 2017

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

@targos let me know if anything else needs changing.

@gibfahn
Copy link
Member Author

gibfahn commented Jan 23, 2017

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

italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 23, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
targos pushed a commit that referenced this pull request Jan 28, 2017
Use assert.strictEqual instead of assert.equal in tests, manually
convert types where necessary.

PR-URL: #10698
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
targos pushed a commit that referenced this pull request Jan 28, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@gibfahn gibfahn restored the assert-strict branch January 29, 2017 22:29
@gibfahn gibfahn deleted the assert-strict branch January 29, 2017 22:32
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Use assert.strictEqual instead of assert.equal in tests, manually
convert types where necessary.

PR-URL: nodejs#10698
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Use assert.strictEqual instead of assert.equal in tests, manually
convert types where necessary.

PR-URL: nodejs#10698
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

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

@bnoordhuis
Copy link
Member

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.

@gibfahn gibfahn restored the assert-strict branch March 9, 2017 08:28
MylesBorins pushed a commit that referenced this pull request Apr 14, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 14, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
@gibfahn gibfahn deleted the assert-strict branch June 3, 2017 14:34
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.