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,repl: use deepStrictEqual for false-y values #6196

Closed

Conversation

Fishrock123
Copy link
Contributor

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

doc,repl

Description of change

Refs: 0b66b8f#commitcomment-17104918, #6192

... Should we just change this throughout all of our testes, except the assert ones themselves?

As a note, I'm not sure how much it matters for this test, but testing false-y values with deepEqual() can easily miss problems.

cc @nodejs/testing

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Apr 14, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Apr 14, 2016

LGTM. And yes, IMO we should make all of the assertions as strict as practical to avoid surprises.

@Fishrock123
Copy link
Contributor Author

And yes, IMO we should make all of the assertions as strict as practical to avoid surprises.

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

@mscdex mscdex added the repl Issues and PRs related to the REPL subsystem. label Apr 14, 2016
@addaleax
Copy link
Member

Oh, yeah, sorry, I just copied from the already-present test. But this sounds good to me, too.

@Trott
Copy link
Member

Trott commented Apr 15, 2016

LGTM.

@Fishrock123 I have a lint rule written and will submit a PR soon. We'll find out if there's opposition or not...

@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

LGTM

1 similar comment
@JungMinu
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

jasnell pushed a commit that referenced this pull request Apr 18, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Landed in 15d970d

@Fishrock123
Copy link
Contributor Author

@jasnell Please avoid landing other collaborator's stuff. I don't think this PR/commit is worthwhile in light of the discussion, thanks.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

@Fishrock123 ... just following the same process I do for everything (and the same process I will continue to follow). There were multiple LGTM's, no obvious indication that it shouldn't land, and green CI. If this is something that you feel should be reverted, please feel free to open a PR reverting it.

Suggestion: we do have an in-progress label. If you feel that one of your PRs is not yet ready to land, attach the in-progress (or an appropriate don't land) label to it and I will skip over it when I'm going through and triaging issues.

@Fishrock123
Copy link
Contributor Author

Historically, for a long time, we have mostly avoided merging other collaborators PRs unless they are npm upgrades, or critical fixes of some sort.

I see that is not stated in our resources and am going to make amends to those.

If this is something that you feel should be reverted, please feel free to open a PR reverting it.

In this case, it's just unnecessary git history. (And personally, as the author of this commit, slightly non-sensical in hindsight.)

@Fishrock123 Fishrock123 deleted the tests-use-deepStrictEqual branch April 19, 2016 05:01
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
This was referenced Apr 21, 2016
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
PR-URL: nodejs#6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
PR-URL: #6196
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants