test: add coverage for AsyncResource constructor #13327
Conversation
mscdex
added
the
async_hooks
|
|
||
| assert.throws(() => { | ||
| return new AsyncResource() | ||
| }, new RegExp('^TypeError: type must be a string with length > 0')); |
There was a problem hiding this comment.
Why not lexical regexes? e.g.
/^TypeError: type must be a string with length > 0$/
|
@jasnell updated |
| const AsyncResource = require('async_hooks').AsyncResource; | ||
| const async_wrap = process.binding('async_wrap'); | ||
|
|
||
| async_wrap.async_hook_fields[async_wrap.constants.kInit] = 1; |
There was a problem hiding this comment.
Mind adding a comment as to why this line is needed?
|
@trevnorris updated |
| async_wrap.async_hook_fields[async_wrap.constants.kInit] = 1; | ||
|
|
||
| assert.throws(() => { | ||
| return new AsyncResource() |
There was a problem hiding this comment.
Does this pass make lint without the semicolon?
| const AsyncResource = require('async_hooks').AsyncResource; | ||
| const async_wrap = process.binding('async_wrap'); | ||
|
|
||
| // without this, there is nothing to do, and the constructor would return immediatly |
|
@Fishrock123 thanks, updated! |
| /* without this, there is nothing to do, | ||
| * and the constructor would return immediately | ||
| */ | ||
| async_wrap.async_hook_fields[async_wrap.constants.kInit] = 1; |
There was a problem hiding this comment.
Refering to #13336 (comment) the logic there in AsyncResource() is bad, so this shouldn't be necessary. If this lands first we need a comment or such that this should be removed.
Also, I'd rather see
async_hooks.createHook({ init() { } }).enable();than manipulating kInit manually.
There was a problem hiding this comment.
makes sense - should I change it to async_hooks.createHook, or remove it entirely and wait for it to be fixed?
There was a problem hiding this comment.
I'd change it and wait for the fix. If there's already a PR w/ the fix then we'll get it in quick. If not then I'll make one. Either way it's a simple fix that shouldn't take more than a day to get landed.
There was a problem hiding this comment.
updated - also, I am happy to submit a PR for the fix as well. let me know if you'd like that
|
|
||
| assert.throws(() => { | ||
| return new AsyncResource(); | ||
| }, new RegExp('^TypeError: type must be a string with length > 0$')); |
There was a problem hiding this comment.
I am not sure @jasnell’s nit was actually addressed… why are these new RegExp() calls? It might be confusing to readers of our tests because we almost exclusively use them to create dynamic regexes that depend on variables.
There was a problem hiding this comment.
No, my comments were not addressed. I would prefer to see the /..../ regex pattern used but I'm fine with this either way.
|
Landed in 52f358b, fixed up nits & linter while landing. Thanks for the PR! |
PR-URL: #13327 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: #13327 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_hooks