docs: stronger suggestion for userland assert by geek · Pull Request #4535 · nodejs/node · GitHub

Credit to @mikeal for language, see #4532 (comment)

LGTM

@mikeal This statement should not be this strong. There is no commonly npm package in existence for asserting _invariants_ in source. At least, not that I know of, can you point to one?

I suggest we make clear that assert is NOT for unit tests, and not point people to non-existent modules for the use-case that assert is a good tool for (invariants).

The assert module provides a simple set of assertion tests that can be used
to test invariants. While the assert module is generally intended for internal use by Node.js itself, it can be used by user code calling require('assert'). For the purposes of unit testing, it is recommended that users of Node.js choose a third party test library from [npm][] instead of relying on assert.

OK, I see, searching for assert on npmjs.org gets you pages of results, none of which are useful to replace assert for invariants, and none of which are a unit test framework (which is probably what the reader wanted).

The usage of assert we want to warn people away from is using it as a unit testing library, such as mocha, tap, and nunit, and none of those are even in the first page of search results when using npmjs.org search.

Of course, I can't find a search term (other than "mocha") that yields mocha in the first page. :-( This is likely because npmjs.org search is unranked, so not so helpful, googling for node test framework works better.

@sam-github I'm a bit confused by your comments and feel that I must be misunderstanding something, so perhaps you could help me understand by rephrasing?

This statement should not be this strong. There is no commonly npm package in existence for asserting _invariants_ in source. At least, not that I know of, can you point to one?
...
OK, I see, searching for assert on npmjs.org gets you pages of results, none of which are useful to replace assert for invariants

Isn't assert, for example, pretty much the same thing? That's the first result I get when I search assert.

FWIW, there's also invariant which is dead simple (basically just like assert()).

and none of which are a unit test framework (which is probably what the reader wanted).

The usage of assert we want to warn people away from is using it as a unit testing library, such as mocha, tap, and nunit,

Do people somehow confuse the assert module as being a testing framework or library? Or do you mean it shouldn't be used for assertions in userland tests period? I think it's fairly commonly used for assertions within a framework like Mocha.

LGTM
I know there's a lack of clarity on which third party assertion libraries to recommend (or even if there are any that are commonly used) but this change is fine, IMHO.

I think Node.js has APIs that are legacy, and we should steer people clear of, but putting phrasing at the top of assert that basically implies "it sucks, we won't improve it because it is locked and is good enough for node, here's a link to an npmjs.org search query with pages of results... good luck" does a disservice to the assert API, which was intended as an analogue of the assert() we all know and love from C, not a unit testing library.

Sure, we should protect people from using assert for things it wasn't intended for, but we shouldn't imply that a perfectly usable Node.js API is to be avoided.

And to put it another way:

What is so special about assert that we are pointing people to npmjs.org and saying there are better things there?

If we are going to do that, then why don't we do it everywhere? Why doesn't https://nodejs.org/api/http.html come with a similar warning, telling people they should be looking on npmjs.org for request, or express/hapi/whatever instead of using the low-level node API? Or cluster, telling people to search npmjs.org for process manager?

Anyhow, I won't belabour this point further: while Node.js has some genuinely unfortunate APIs that should be avoided if not fixed, I don't think assert is one of them, so making assert out to be an API to be avoided does it a disservice.

/to @cjihrig, and @mikeal (who suggested this change in the first place, and should weigh in to why he thinks its helpful)

@sam-github I am fine either way... I just want to make whatever the decision is clear. At the moment, the wording on https://nodejs.org/api/assert.html makes it sound like people should avoid using assert, if thats the case I want to make it clearer. If not, then we should remove the suggestion to avoid assert.

The future of assert came up in a CTC meeting. The notes aren't very detailed (could probably go back and listen to the audio), but the resolution was that assert should only be used for Node's internal testing. Internal modules didn't exist when assert was created, or it would have likely been an internal module.

IIRC a big issue was breaking changes in assert introduced by trying to keep up with new language features and edge cases (comparing Error objects, for example).

I completely agree with @sam-github that assert is not a module that needs to be avoided. I think the decision was more a reluctance of the core team to maintain an assertion library, which can be opinionated and easily implemented in userland.

Regarding this issue, I think we should:

Remove the link out to npm search results.
Mention that assert is meant for Node's own internal testing.
Mention that it is definitely not a testing framework, and is not intended as a general purpose assertion library.

I'd be good with @cjihrig's suggestions.

Credit to @cjihrig for updated language.

LGTM

LGTM, thanks for humouring me on this.

Minor nit... I'd like to avoid starting sentences with a lower-cased module name, especially two sentences in a row... Perhaps reword the las two sentences to:

The module is intended for internal use by Node.js, but can be used in application 
code via `require('assert')`. However, `assert` is not a testing framework, and is not 
intended to be used as a general purpose assertion library.

Minor nit, otherwise LGTM

Subsystem prefix should probably be doc: instead of docs: to be consistent with most other doc commits.

LGTM

Fixes: #4532 PR-URL: #4535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Landed in d7c5110

Fixes: #4532 PR-URL: #4535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

Fixes: nodejs#4532 PR-URL: nodejs#4535 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

docs: stronger suggestion for userland assert

ed29a58

mscdex added assert Issues and PRs related to the assert subsystem. doc Issues and PRs related to the documentations. labels Jan 5, 2016

sam-github reviewed Jan 5, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Jan 5, 2016

geek added 2 commits January 5, 2016 16:24

Update assert.markdown

07d5d48

Used wording from @cjihrig

e6da014

Credit to @cjihrig for updated language.

jasnell reviewed Jan 6, 2016
View reviewed changes

With feedback from @jasnell

581a3b1

cjihrig closed this Jan 11, 2016

MylesBorins mentioned this pull request Jan 11, 2016

V5.4.1 propose #4626

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 13, 2016

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

docs: stronger suggestion for userland assert by geek · Pull Request #4535 · nodejs/node · GitHub

Uh oh!

Uh oh!

docs: stronger suggestion for userland assert - #4535

docs: stronger suggestion for userland assert#4535geek wants to merge 4 commits intonodejs:masternodejs/node:masterfrom geek:issue4532geek/node:issue4532Copy head branch name to clipboard

Conversation

geek commented Jan 5, 2016

Uh oh!

evanlucas commented Jan 5, 2016

Uh oh!

sam-github Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

sam-github Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

jmm Jan 5, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 5, 2016

Uh oh!

sam-github commented Jan 5, 2016

Uh oh!

geek commented Jan 5, 2016

Uh oh!

cjihrig commented Jan 5, 2016

Uh oh!

sam-github commented Jan 5, 2016

Uh oh!

cjihrig commented Jan 5, 2016

Uh oh!

sam-github commented Jan 6, 2016

Uh oh!

jasnell Jan 6, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 6, 2016

Uh oh!

mscdex commented Jan 6, 2016

Uh oh!

silverwind commented Jan 9, 2016

Uh oh!

cjihrig commented Jan 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

docs: stronger suggestion for userland assert#4535
geek wants to merge 4 commits into
nodejs:masterfrom
geek:issue4532