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

test: added `common.mustCallAtLeast` #12935

Merged
merged 0 commits into from May 19, 2017

Conversation

@refack
Copy link
Member

commented May 9, 2017

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

@refack refack self-assigned this May 9, 2017

@refack refack requested review from jasnell and addaleax May 9, 2017

test/common/index.js Outdated
/**
* @function mustCall
* @param fn "The code that must be called
* @param {?number|string} expected "Expected times to be called, or '+' for more then 0. Default = 1"

This comment has been minimized.

Copy link
@addaleax

addaleax May 9, 2017

Member

typo: then → than

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

test/common/index.js Outdated
* @returns {Function}
* @export
*/
exports.mustCall = function mustCall(fn, expected) {

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

Can you undo this block of changes.

This comment has been minimized.

Copy link
@refack

refack May 9, 2017

Author Member

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

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

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.

test/parallel/test-test-common.js Outdated
@@ -0,0 +1,16 @@
'use strict';

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 9, 2017

Contributor

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

This comment has been minimized.

Copy link
@refack

refack May 9, 2017

Author Member

didn't see that one...

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

I think this needs a documentation update.

@refack refack referenced this pull request May 9, 2017
3 of 3 tasks complete
@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

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

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

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

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

#12930 (comment)

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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.

@jasnell
jasnell approved these changes May 9, 2017
Copy link
Member

left a comment

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

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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

@mscdex

This comment has been minimized.

Copy link
Contributor

commented May 9, 2017

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

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

@mscdex I was thinking about that.
Will do.

@Trott
Copy link
Member

left a comment

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

test/common/README.md Outdated
fail.
Returns a function that calls `fn`. 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.

This comment has been minimized.

Copy link
@Trott

Trott May 9, 2017

Member

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.

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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

This comment has been minimized.

Copy link
Member

commented May 9, 2017

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

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

Replace magic '+' with mustCallAtLeast PTAL

@refack refack force-pushed the refack:mustCall+ branch May 9, 2017

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

@refack

This comment has been minimized.

Copy link
Member Author

commented May 9, 2017

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

@Trott

This comment has been minimized.

Copy link
Member

commented May 10, 2017

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.

@refack refack force-pushed the refack:mustCall+ branch May 19, 2017

@refack

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

@refack refack closed this May 19, 2017

@refack refack force-pushed the refack:mustCall+ branch to fccc0bf May 19, 2017

@refack

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

landed in fccc0bf

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

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

@jasnell jasnell referenced this pull request May 28, 2017
@refack refack referenced this pull request Jun 3, 2017
4 of 4 tasks complete

@refack refack removed their assignment Jun 12, 2017

@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

Should this land on v6.x?

@refack

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2017

Should this land on v6.x?

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

@refack

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2017

#14327 depends on this

@refack refack referenced this pull request Jul 17, 2017
2 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Aug 14, 2017
test: add mustCallAtLeast
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 added a commit that referenced this pull request Aug 16, 2017
test: add mustCallAtLeast
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 MylesBorins referenced this pull request Aug 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.