docs: stronger suggestion for userland assert #4535
Conversation
mscdex
added
assert
|
LGTM |
| code calling `require('assert')`. | ||
| to test invariants and implement unit tests. It is recommended that users of | ||
| Node.js choose a third party assertion library from [npm][] instead of relying | ||
| on `assert`. That being said, `assert` can be used by user code calling |
There was a problem hiding this comment.
@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
assertmodule provides a simple set of assertion tests that can be used
to test invariants. While theassertmodule is generally intended for internal use by Node.js itself, it can be used by user code callingrequire('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 onassert.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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 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 IIRC a big issue was breaking changes in I completely agree with @sam-github that Regarding this issue, I think we should:
|
|
I'd be good with @cjihrig's suggestions. |
Credit to @cjihrig for updated language.
|
LGTM |
|
LGTM, thanks for humouring me on this. |
| The `assert` module provides a simple set of assertion tests that can be used to | ||
| test invariants. `assert` is intended for internal use by Node.js, but can be | ||
| used in application code via `require('assert')`. `assert` is not a testing | ||
| framework, and is not intended to be used as a general purpose assertion library. | ||
|
|
There was a problem hiding this comment.
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 |
|
LGTM |
|
Landed in d7c5110 |
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>
Fixes #4532
Credit to @mikeal for language, see #4532 (comment)