doc: add information about Assert behavior and maintenance #3330

Trott · 2015-10-12T21:00:32Z

Doc that:

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

cjihrig · 2015-10-12T21:03:46Z

LGTM, assuming we're going with the docs only approach.

domenic · 2015-10-12T21:04:31Z

doc/api/assert.markdown

+
+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`


missing "example"

Fixed the typo. Thanks!

domenic · 2015-10-12T21:04:41Z

doc/api/assert.markdown

@@ -2,8 +2,9 @@

    Stability: 2 - Stable


I wonder if we should push this to 4 - locked.

That would probably be a TSC decision, yes?

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

Yeah, I remember that too. Might be good to run this by the TSC in general... I'll tag it tsc-agenda.

Fishrock123 · 2015-10-13T05:10:54Z

doc/api/assert.markdown

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


I'm not actually sure I agree with this, since it already exists.

Might as well just deprecate it then, no?

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

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 · 2015-10-13T05:11:40Z

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 · 2015-10-13T05:41:24Z

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

Ref: nodejs#3124 Ref: nodejs#3122

Trott · 2015-10-16T22:08:48Z

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 · 2015-10-17T04:21:28Z

Still LGTM

Trott · 2015-10-18T21:15:28Z

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 · 2015-10-18T21:24:39Z

LGTM

rvagg · 2015-10-19T00:26:35Z

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>

Trott · 2015-10-19T18:50:54Z

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>

jasnell · 2015-10-26T17:18:33Z

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>

DanLipsitt · 2015-12-04T19:59:33Z

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 · 2015-12-04T20:06:45Z

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 · 2015-12-04T22:00:43Z

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.

Trott mentioned this pull request Oct 12, 2015

lib: generalize handling in assert.deepEqual() #3124

Closed

domenic reviewed Oct 12, 2015
View reviewed changes

Trott force-pushed the assert-doc branch from e0deef9 to 40f27e4 Compare October 12, 2015 21:04

domenic reviewed Oct 12, 2015
View reviewed changes

domenic added the tsc-agenda label Oct 12, 2015

brendanashworth added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Oct 12, 2015

Fishrock123 reviewed Oct 13, 2015
View reviewed changes

rvagg mentioned this pull request Oct 14, 2015

Node Foundation TSC Meeting 2015-10-14 #3363

Closed

Trott added 2 commits October 16, 2015 14:54

doc: add information about Assert behavior and maintenance

4075348

Ref: nodejs#3124 Ref: nodejs#3122

fixup

cd96fc7

Trott force-pushed the assert-doc branch from 40f27e4 to cd96fc7 Compare October 16, 2015 21:54

Trott added 2 commits October 16, 2015 15:00

fixup: Locked

88b8d21

fixup: pronouns

3b5984c

Trott added the lts-watch-v4.x label Oct 19, 2015

Trott mentioned this pull request Oct 19, 2015

assert.deepEqual & post-ES5 Data Types e.g. Map, Set, Iterables, etc #2309

Closed

Trott closed this Oct 19, 2015

rvagg mentioned this pull request Oct 21, 2015

Release proposal: v5.0.0 #3466

Merged

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 26, 2015

rvagg mentioned this pull request Oct 28, 2015

Preparing v4.2.2 LTS Maintenance nodejs/Release#50

Closed

jasnell removed the land-on-v4.x label Nov 3, 2015

ChALkeR removed the tsc-agenda label Nov 11, 2015

inikulin mentioned this pull request May 26, 2016

[docs] GS added DevExpress/testcafe#534

Merged

Trott deleted the assert-doc branch January 13, 2022 22:29

snyk-bot mentioned this pull request Apr 24, 2023

[Snyk] Security upgrade lerna from 3.22.1 to 5.5.2 aliscco/alisco-node#102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: add information about Assert behavior and maintenance #3330

doc: add information about Assert behavior and maintenance #3330

Trott commented Oct 12, 2015

cjihrig commented Oct 12, 2015

domenic Oct 12, 2015

Trott Oct 12, 2015

domenic Oct 12, 2015

Trott Oct 12, 2015

Trott Oct 12, 2015

domenic Oct 12, 2015

Fishrock123 Oct 13, 2015

Trott Oct 13, 2015

aheckmann Oct 14, 2015

Fishrock123 commented Oct 13, 2015

Trott commented Oct 13, 2015

Trott commented Oct 16, 2015

cjihrig commented Oct 17, 2015

Trott commented Oct 18, 2015

indutny commented Oct 18, 2015

rvagg commented Oct 19, 2015

Trott commented Oct 19, 2015

jasnell commented Oct 26, 2015

DanLipsitt commented Dec 4, 2015

tflanagan commented Dec 4, 2015

Trott commented Dec 4, 2015

doc: add information about Assert behavior and maintenance #3330

doc: add information about Assert behavior and maintenance #3330

Conversation

Trott commented Oct 12, 2015

cjihrig commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Oct 13, 2015

Trott commented Oct 13, 2015

Trott commented Oct 16, 2015

cjihrig commented Oct 17, 2015

Trott commented Oct 18, 2015

indutny commented Oct 18, 2015

rvagg commented Oct 19, 2015

Trott commented Oct 19, 2015

jasnell commented Oct 26, 2015

DanLipsitt commented Dec 4, 2015

tflanagan commented Dec 4, 2015

Trott commented Dec 4, 2015