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

doc: add information about Assert behavior and maintenance #3330

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 12, 2015

Doc that:

  • assert is for Node.js testing itself and not intended for general use
  • deepEqual() only considers enumerable properties

Ref: #3124
Ref: #3122

@cjihrig
Copy link
Contributor

cjihrig commented Oct 12, 2015

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`
Copy link
Contributor

Choose a reason for hiding this comment

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

missing "example"

Copy link
Member Author

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

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.

Copy link
Member Author

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?

Copy link
Member Author

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

Copy link
Contributor

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.

@brendanashworth brendanashworth added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Oct 12, 2015
`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.
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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.

@Fishrock123
Copy link
Contributor

FWIW I think deepStrictEqual should just do all the things so no one will ever complain again it doesn't check deep enough or strict enough.

@Trott
Copy link
Member Author

Trott commented Oct 13, 2015

@Fishrock123 Regarding deepStrictEqual(), if it makes you feel even better about #3124...

Node 4.2.0:

// Does not throw. Wat?!?!
assert.deepStrictEqual(Error('a'), Error('b'));

With #3124:

// Totally throws an AssertionError. Hooray!
assert.deepStrictEqual(Error('a'), Error('b'));

I suppose I could try to revise the code so that it only does this for deepStrictEqual() and not deepEqual(), but if we're going to do a semver-major change anyway, why not get both of them behaving like the user no doubt expects?

@Trott
Copy link
Member Author

Trott commented Oct 16, 2015

Now that the TSC has elected to lock the AssertAPI, I've updated this PR to reflect that.

  • @cjihrig Still looks good to you?
  • @Fishrock123 Post-TSC, are you now comfortable with the text content or are you still not sure you can get behind it? Is this LGTM-able?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 17, 2015

Still LGTM

@Trott
Copy link
Member Author

Trott commented Oct 18, 2015

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.

@indutny
Copy link
Member

indutny commented Oct 18, 2015

LGTM

@rvagg
Copy link
Member

rvagg commented Oct 19, 2015

lgtm

Trott added a commit to Trott/io.js that referenced this pull request Oct 19, 2015
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>
@Trott
Copy link
Member Author

Trott commented Oct 19, 2015

Landed in f875c73

@Trott Trott closed this Oct 19, 2015
Trott added a commit that referenced this pull request Oct 21, 2015
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>
@rvagg rvagg mentioned this pull request Oct 21, 2015
Trott added a commit that referenced this pull request Oct 26, 2015
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>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 4023c7d

Trott added a commit that referenced this pull request Oct 29, 2015
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>
@DanLipsitt
Copy link

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.

@tflanagan
Copy link
Contributor

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)

@Trott
Copy link
Member Author

Trott commented Dec 4, 2015

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 AssertionError which is clearly wrong and surprising:

assert.deepStrictEqual(Error('foo'), Error('bar'));

These sorts of problems would likely be fixed in a userland assertion library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.