-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
doc: add information about Assert behavior and maintenance #3330
Conversation
LGTM, assuming we're going with the docs only approach. |
|
||
This only considers enumerable properties. It does not test object prototypes, | ||
attached symbols, or non-enumerable properties. This can lead to some | ||
potentially surprising results. For this does not throw an `AssertionError` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing "example"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the typo. Thanks!
@@ -2,8 +2,9 @@ | |||
|
|||
Stability: 2 - Stable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should push this to 4 - locked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would probably be a TSC decision, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, 3 is locked. Also, I thought there was talk about just getting rid of the stability index stuff altogether in the project. But I can't seem to find that right now. Maybe I imagined it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I remember that too. Might be good to run this by the TSC in general... I'll tag it tsc-agenda.
`require('assert')`. | ||
This module is used so that Node.js can test itself. You can access it with | ||
`require('assert')`. However, it is recommended that you use a userland | ||
assertion library instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually sure I agree with this, since it already exists.
Might as well just deprecate it then, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, at least a docs-only deprecation if that's the way it's supposed to be. @domenic tagged this tsc-agenda
, so maybe there can be consensus at the next TSC meeting on whether or not assert
is a general use library or really just intended for use in the Node.js project. If there's general agreement on that, one way or the other, everything flows naturally from it. (Internal use? Doc update only. General purpose? Surely the current surprising behavior should be considered a defect, albeit one that should be fixed carefully and not rushed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using assert
for years because its publicly documented and serves all my needs. The "node only" route ship sailed a long time ago IMO.
FWIW I think |
@Fishrock123 Regarding Node 4.2.0:
With #3124:
I suppose I could try to revise the code so that it only does this for |
Now that the TSC has elected to lock the
|
Still LGTM |
I'll land this in about 24 hours unless someone has an objection. A second LGTM from a @nodejs/tsc member would be appreciated, just to cement the "Yes, we concluded this API should be Locked" aspect. |
LGTM |
lgtm |
Assert is now locked. Userland alternatives should be used. Assert is for testing Node.js itself. Document potentially surprising use of enumerable properties only in deep equality assertions. Ref: nodejs#3124 Ref: nodejs#3122 PR-URL: nodejs#3330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Landed in f875c73 |
Assert is now locked. Userland alternatives should be used. Assert is for testing Node.js itself. Document potentially surprising use of enumerable properties only in deep equality assertions. Ref: #3124 Ref: #3122 PR-URL: #3330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Assert is now locked. Userland alternatives should be used. Assert is for testing Node.js itself. Document potentially surprising use of enumerable properties only in deep equality assertions. Ref: #3124 Ref: #3122 PR-URL: #3330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>
Landed in v4.x-staging in 4023c7d |
Assert is now locked. Userland alternatives should be used. Assert is for testing Node.js itself. Document potentially surprising use of enumerable properties only in deep equality assertions. Ref: #3124 Ref: #3122 PR-URL: #3330 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Rod Vagg <rod@vagg.org>
I've been looking for an explanation of why it's bad to use node's assert outside node itself. I understand the message that it isn't intended for that purpose, but is there anything about it that makes it actually bad? I see it used a fair amount in testing tutorials around the web. |
No, there is nothing that makes it bad for external use, and the API is locked so you can be assured it won't change (minus some huge issue down the road that no one can foresee) |
There are indeed some issues with it, and the reason you shouldn't use it is because those issues will not be fixed. For example, this does not throw an
These sorts of problems would likely be fixed in a userland assertion library. |
Doc that:
assert
is for Node.js testing itself and not intended for general usedeepEqual()
only considers enumerable propertiesRef: #3124
Ref: #3122