test: added `common.mustCallAtLeast` by refack · Pull Request #12935 · nodejs/node · GitHub

added common.mustCallAtLeast for calling more than minimum times.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

test

typo: then → than

CI: https://ci.nodejs.org/job/node-test-commit/9765/
/cc @nodejs/testing

Can you undo this block of changes.

the JSDocs?
It's usefull for people with IDEs that can parse these (VSCode / Webstorm)

Other than lib/punycode.js which is vendored, there are only 9 @param matches in all of lib, src, and test. It's more consistent to drop them.

This code should live in test/parallel/test-common.js I think.

didn't see that one...

I think this needs a documentation update.

Docs added, nit's addressed. @cjihrig I'd rather keep the JSDocs, unless you have a strong objection.

I wouldn't say strong objection, but definitely -1.

I wouldn't say strong objection, but definitely -1.

Gone.
I'll do a PR with JSDocs for the whole file once 😉

Could you point to somewhere that this would be better suited for rather than the explicit number of calls?

I generally think this is a bad idea because if something is being called more times than you expect you've probably got a bug.

Could you point to somewhere that this would be better suited for rather than the explicit number of calls?

#12930 (comment)

I generally think this is a bad idea because if something is being called more times than you expect you've probably got a bug.

I tend to agree, but some things are either OS dependent or non deterministic. In those cases we tend to skip the mustCall all together which IMHO is worse.
I agree this should be used sparingly, and usage should receive justification in a comment.

I've been wanting this for a while. Once @cjihrig is happy with it, I'm LGTM

-1 I don't think it is a good idea to reuse the same parameter (or even the same function) for this.

@Fishrock123 ... this came up with regards to adding mustCall() around a data event handler, which could be reasonably called any number of times.

@mscdex ... the other option I had in mind was a common.mustCallAtLeast(n) method that would allow a minimum number to be specified.

@jasnell I would much prefer a separate method so that the intention is clear.

@jasnell I would much prefer a separate method so that the intention is clear.

@mscdex I was thinking about that.
Will do.

-0 from me. I dislike magic values. I prefer that things in common have simple, self-explanatory, and intuitively obvious interfaces. This adds more cognitive overhead, especially for newcomers.

Nit: This documentation change results in a confusing run-on sentence:

If the returned function has not been called exactly expected number of times, or at least once if
expected === '+', when the test is complete, then the test will fail.

Would be better as something like:

If the returned function has not been called exactly expected number of times when the test is complete, then the test will fail. If expected is '+'`, then the test must run at least once.

the other option I had in mind was a common.mustCallAtLeast(n)

+1 to that

-0 from me. I dislike magic values

@Trott thoughts on @jasnell's proposed common.mustCallAtLeast(2)?

@Trott thoughts on @jasnell's proposed common.mustCallAtLeast(2)?

Eh, still pretty +/-0 on it. I think there's enough API surface area in common and we quickly reach a point of diminishing returns.

Moreover, I'm not really sure this is solving a very common problem. I imagine it's mostly for data callbacks in tests that are called once but could theoretically be called more than once. However, those almost never need to actually be wrapped to make sure they're called because they usually have something like data += chunk and then the value of data is checked somewhere, so you don't really need to check that the callback was called. The test will fail if it wasn't. (Will there be a small number of empty callbacks for data that we want to check? Sure. Is it worth adding a whole other function to the common monolith for it? I doubt it.)

But if others feel this has big value, I won't stop it.

Replace magic '+' with mustCallAtLeast PTAL

P.S. why don't we adopt something like sinon.
I'm a big believer that better tooling begets better tests.

P.S. why don't we adopt something like sinon.

Biggest reason is probably because no one's bothered to do it yet.

That said, a PR adding sinon very well might not get accepted. Speaking only for myself, I have grown wary of things that raise the barrier to entry for people working on tests. So to the extent that we avoid extra tools and unneeded abstractions in the common module, I'm happy. But that said, if the benefit would be considerable and obvious, I'd be for it.

Others in @nodejs/testing might have other opinions.

Pre land CI: https://ci.nodejs.org/job/node-test-commit/10013/

landed in fccc0bf

Should this land on v6.x?

Should this land on v6.x?

Sure. It's a semver-minor change in the test harness.

#14327 depends on this

PR-URL: #12935 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label May 9, 2017

refack self-assigned this May 9, 2017

refack requested review from addaleax and jasnell May 9, 2017 21:25

addaleax approved these changes May 9, 2017

View reviewed changes

cjihrig requested changes May 9, 2017

View reviewed changes

refack mentioned this pull request May 9, 2017

test: improve numerous http tests #12930

Closed

3 tasks

jasnell approved these changes May 9, 2017

View reviewed changes

Trott reviewed May 9, 2017

View reviewed changes

refack force-pushed the mustCall+ branch from 95517cc to a6bb177 Compare May 9, 2017 23:30

refack changed the title test: allow mustCall '+' as expected test: added common.mustCallAtLeast May 9, 2017

refack mentioned this pull request May 10, 2017

create public nodejs/collaborators repo nodejs/TSC#243

Closed

refack force-pushed the mustCall+ branch from 141dc1f to a5b2396 Compare May 19, 2017 17:39

refack closed this May 19, 2017

refack force-pushed the mustCall+ branch from a5b2396 to fccc0bf Compare May 19, 2017 19:24

refack deleted the mustCall+ branch May 19, 2017 19:24

refack merged commit fccc0bf into nodejs:master May 19, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

refack mentioned this pull request Jun 3, 2017

test: add hijackStdout and hijackStderr #13439

Closed

4 tasks

refack removed their assignment Jun 12, 2017

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 17, 2017

refack mentioned this pull request Jul 17, 2017

[v6.x backport] test: harden test-dgram-bind-shared-ports #14327

Closed

3 tasks

MylesBorins added land-on-v6.x and removed lts-watch-v6.x semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 14, 2017

MylesBorins pushed a commit that referenced this pull request Aug 14, 2017

test: add mustCallAtLeast

ce86b8e

PR-URL: #12935 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017

test: add mustCallAtLeast

f578c9b

PR-URL: #12935 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

test: added `common.mustCallAtLeast` by refack · Pull Request #12935 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: added common.mustCallAtLeast - #12935

test: added common.mustCallAtLeast#12935refack merged 0 commit intonodejs:masternodejs/node:masterfrom refack:mustCall+refack/node:mustCall+Copy head branch name to clipboard

Conversation

refack commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax May 9, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented May 9, 2017

Uh oh!

cjihrig May 9, 2017

Choose a reason for hiding this comment

Uh oh!

refack May 9, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig May 9, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig May 9, 2017

Choose a reason for hiding this comment

Uh oh!

refack May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

cjihrig commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

Fishrock123 commented May 9, 2017

Uh oh!

Fishrock123 commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 9, 2017

Uh oh!

mscdex commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott May 9, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn commented May 9, 2017

Uh oh!

Trott commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

refack commented May 9, 2017

Uh oh!

Trott commented May 10, 2017

Uh oh!

refack commented May 19, 2017

Uh oh!

refack commented May 19, 2017

Uh oh!

test: added common.mustCallAtLeast#12935
refack merged 0 commit into
nodejs:masterfrom
refack:mustCall+

refack commented May 9, 2017 •
edited
Loading

refack May 9, 2017 •
edited
Loading

mscdex commented May 9, 2017 •
edited
Loading