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.

Sign up for GitHub

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

Jump to bottom

test: increase util.callbackify() coverage #13705

Merged
merged 1 commit into from
Jun 19, 2017
Merged

test: increase util.callbackify() coverage #13705

merged 1 commit into from
Jun 19, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 15, 2017

This commit adds coverage for util.callbackify() type checking.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 15, 2017
refack
refack reviewed Jun 15, 2017
View reviewed changes
test/parallel/test-util-callbackify.js

{
// Verify that non-function inputs throw.
['foo', null, undefined, false, 0, {}, Symbol(), []].forEach((value) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nit: could use values.filter(v => typeof v !== 'function')

refack
refack reviewed Jun 15, 2017
View reviewed changes
test/parallel/test-util-callbackify.js
}, common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "last argument" argument must be of type function'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this output is a tiny bit unfortunate 🤷‍♂️ I should have covered this

@mscdex mscdex added the util Issues and PRs related to the built-in util module. label Jun 15, 2017
refack pushed a commit to refack/node that referenced this pull request Jun 17, 2017
@cjihrig @refack
test: increase util.callbackify() coverage
ace4ade 
This commit adds coverage for util.callbackify() type checking.

PR-URL: nodejs#13705
@refack
Copy link
Contributor

refack commented Jun 17, 2017

Ref: #12712

@refack refack mentioned this pull request Jun 17, 2017
Closed
4 tasks
BridgeAR
BridgeAR approved these changes Jun 17, 2017
View reviewed changes
test/parallel/test-util-callbackify.js

{
async function asyncFn() {
return await Promise.resolve(42);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the return itself is already sufficient as the return value is implicitly wrapped in a promise. Therefore the await can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but that's the way to other tests are written (e.g. https://github.com/nodejs/node/blob/master/test/parallel/test-util-callbackify.js#L31)
The rationale was to make them actually async in timing, not just in signature.

@refack
Copy link
Contributor

refack commented Jun 18, 2017

@cjihrig I would like to bundle this with the other callbackify code to port to node8. IMHO this could land.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 19, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/8735/

EDIT: Linter failed, so CI again: https://ci.nodejs.org/job/node-test-pull-request/8740/

@cjihrig
test: increase util.callbackify() coverage
471e88f 
This commit adds coverage for util.callbackify() type checking.

PR-URL: nodejs#13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@cjihrig cjihrig merged commit 471e88f into nodejs:master Jun 19, 2017
@cjihrig cjihrig deleted the callbackify branch June 19, 2017 18:41
addaleax pushed a commit that referenced this pull request Jun 20, 2017
@cjihrig @addaleax
test: increase util.callbackify() coverage
b8271a4 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@addaleax addaleax mentioned this pull request Jun 21, 2017
Merged
addaleax pushed a commit that referenced this pull request Jun 21, 2017
@cjihrig @addaleax
test: increase util.callbackify() coverage
d21e4f1 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax pushed a commit that referenced this pull request Jun 24, 2017
@cjihrig @addaleax
test: increase util.callbackify() coverage
93440f7 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 29, 2017
@cjihrig @rvagg
test: increase util.callbackify() coverage
e1006ca 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@cjihrig @addaleax
test: increase util.callbackify() coverage
17c1942 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@cjihrig @addaleax
test: increase util.callbackify() coverage
bf22514 
This commit adds coverage for util.callbackify() type checking.

PR-URL: #13705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorins
Copy link
Contributor

setting as don't land, will likely need to be revisited if we decide to backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@cjihrig @refack @MylesBorins @jasnell @benjamingr @tniessen @BridgeAR @mscdex @nodejs-github-bot