test: increase coverage of async_hooks #13336

DavidCai1111 · 2017-05-31T16:32:33Z

increase coverage of lib/async_hooks.js
rename test/async_hooks/test-embedder.api.async-event.*.js to test/async_hooks/test-embedder.api.async-resource.*.js

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)

async_hooks

kunalspathak · 2017-05-31T17:34:54Z

Unrelated to this PR, but I noticed that we async-hooks test don't run for windows. Any reason why? @trevnorris , @addaleax .

addaleax · 2017-05-31T17:36:44Z

test/async-hooks/test-run-in-async-id-scope.js

+
+async_hooks.runInAsyncIdScope(asyncId, common.mustCall(() => {
+  assert.strictEqual(async_hooks.currentId(), asyncId);
+}));


I think this test can go into parallel (it doesn’t use any of the async hook test helpers here)

@addaleax Done, PTAL :=)

AndreasMadsen · 2017-06-01T07:42:06Z

Unrelated to this PR, but I noticed that we async-hooks test don't run for windows. Any reason why? @trevnorris , @addaleax .

Sigh, I guess it is because it doesn't appear in https://github.com/nodejs/node/blob/master/vcbuild.bat#L68L82

AndreasMadsen · 2017-06-01T07:44:02Z

test/async-hooks/test-emit-init.js

+const expectedId = async_hooks.newUid();
+const expectedTriggerId = async_hooks.newUid();
+const expectedType = 'test_emit_init_type';
+const expectedResource = Symbol('test_emit_init_resource');


A resource must be an object.

@AndreasMadsen Updated, PTAL :=)

kunalspathak · 2017-06-01T17:38:36Z

Sigh, I guess it is because it doesn't appear in https://github.com/nodejs/node/blob/master/vcbuild.bat#L68L82

Yep, that was my question 😄 . Why is it not present in vcbuild.bat?

AndreasMadsen · 2017-06-01T21:16:58Z

/cc @refack regarding vcbuild.bat

refack · 2017-06-01T21:19:10Z

/cc @refack regarding vcbuild.bat

AFAIK there's no reason except nobody bothered to add it.

refack · 2017-06-01T21:46:50Z

Cross-ref: #13378

DavidCai1111 · 2017-06-02T03:01:51Z

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

AndreasMadsen

LGTM, I don't think we need to wait for the windows PR to be merged. These are mostly pure JS logic related tests.

trevnorris

Thanks for doing all this. Test coverage for async_hooks is important and happy to see more of it coming in. I have two suggestions and one of your test additions made me aware of a flaw in the AsyncResource() constructor logic that needs to be fixed.

trevnorris · 2017-06-02T09:09:38Z

test/async-hooks/test-callback-error.js

+switch (process.argv[2]) {
+  case 'test_init_callback':
+    initHooks({
+      oninit: common.mustCall(() => { throw new Error('test_init_callback'); })


There's the problem where if test_init_callback is never hit then the common.mustCall() will never run. Instead you'll need to do it like this:

const oninitMustCall = common.mustCall(() => { throw new Error('test_init_callback'); }); switch (process.argv[2]) { case 'test_init_callback': initHooks({ oninit: oninitMustCall }).enable();

😕 But IMO the actual intention here is just to ensure that the common.mustCall() will run only when the test_init_callback case is hit? Every switch case will be hit in different process because of spawnSync()s below.

But IMO the actual intention here is just to ensure that the common.mustCall() will run only when the test_init_callback case is hit?

Whoops. Missed that. You're correct.

trevnorris · 2017-06-02T09:25:32Z

test/async-hooks/test-embedder.api.async-resource.js

@@ -9,9 +9,21 @@ const { AsyncResource } = async_hooks;
 const initHooks = require('./init-hooks');
 const { checkInvocations } = require('./hook-checks');

+// Verify that if there is no registered hook, then those invalid parameters
+// won't be checked.
+assert.doesNotThrow(() => new AsyncResource());


I screwed up the logic here. It shouldn't return early before the arguments are checked. Otherwise it'll cause an async hook stack corruption. Here's the minimal test case:

new (require('async_hooks').AsyncResource)().emitBefore();

Here's the diff for the fix. Feel free to add it to this PR along with the above test case:

diff --git a/lib/async_hooks.js b/lib/async_hooks.js index d5d5407..aec73ed 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -205,15 +205,15 @@ class AsyncResource { } this[trigger_id_symbol] = triggerId; - // Return immediately if there's nothing to do. - if (async_hook_fields[kInit] === 0) - return; - if (typeof type !== 'string' || type.length <= 0) throw new TypeError('type must be a string with length > 0'); if (!Number.isSafeInteger(triggerId) || triggerId < 0) throw new RangeError('triggerId must be an unsigned integer'); + // Return immediately if there's nothing to do. + if (async_hook_fields[kInit] === 0) + return; + processing_hook = true; for (var i = 0; i < active_hooks_array.length; i++) { if (typeof active_hooks_array[i][init_symbol] === 'function') {

OK, i'll do it in this PR :=)

@trevnorris Done, added the diff to this PR, PTAL :=)

trevnorris · 2017-06-02T09:27:03Z

test/async-hooks/test-emit-before-after.js

+
+switch (process.argv[2]) {
+  case 'test_invalid_async_id':
+    async_hooks.emitBefore(-1);


To make sure these run I' possibly do this:

const emitBeforeMustCall = common.mustCall(() => async_hooks.emitBefore(-1)); switch (process.argv[2]) { case 'test_invalid_async_id': emitBeforeMustCall();

refack · 2017-06-04T23:32:11Z

Needs a rebase (just tried locally it goes smooth)

DavidCai1111 · 2017-06-05T08:34:08Z

@refack I rebased it locally and the result shows that it goes very smooth 😃

refack · 2017-06-05T16:14:17Z

@refack I rebased it locally and the result shows that it goes very smooth 😃

Apparently GitHub is a whiny little ...

DavidCai1111 · 2017-06-06T13:10:15Z

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

DavidCai1111 · 2017-06-06T13:11:58Z

@refack Now no conflicts checking and CI both seems good ...

DavidCai1111 · 2017-06-06T13:18:04Z

@trevnorris PTAL

trevnorris · 2017-06-06T16:16:55Z

@DavidCai1993 Much thanks for the fix.

DavidCai1111 · 2017-06-07T03:07:44Z

Landed in 35353a4

PR-URL: #13336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: nodejs#13378 Refs: nodejs#13340 Refs: nodejs#13336 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com>

PR-URL: #13378 Refs: #13340 Refs: #13336 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com>

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels May 31, 2017

addaleax reviewed May 31, 2017

View reviewed changes

DavidCai1111 force-pushed the test/async-hook-coverage branch 2 times, most recently from 385a979 to 629f23b Compare June 1, 2017 03:27

AndreasMadsen reviewed Jun 1, 2017

View reviewed changes

DavidCai1111 force-pushed the test/async-hook-coverage branch from 629f23b to d44fb25 Compare June 1, 2017 08:30

jasnell approved these changes Jun 1, 2017

View reviewed changes

refack mentioned this pull request Jun 1, 2017

build,windows: merge test suite groups #13378

Merged

4 tasks

AndreasMadsen approved these changes Jun 2, 2017

View reviewed changes

trevnorris suggested changes Jun 2, 2017

View reviewed changes

trevnorris mentioned this pull request Jun 2, 2017

test: add coverage for AsyncResource constructor #13327

Closed

3 tasks

test: increase coverage of async_hooks

69098bb

DavidCai1111 force-pushed the test/async-hook-coverage branch from d44fb25 to 69098bb Compare June 5, 2017 07:42

AndreasMadsen approved these changes Jun 5, 2017

View reviewed changes

trevnorris approved these changes Jun 6, 2017

View reviewed changes

DavidCai1111 closed this Jun 7, 2017

DavidCai1111 added a commit that referenced this pull request Jun 7, 2017

test: increase coverage of async_hooks

35353a4

PR-URL: #13336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell pushed a commit that referenced this pull request Jun 7, 2017

test: increase coverage of async_hooks

68e06e6

PR-URL: #13336 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Andreas Madsen <amwebdk@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

refack mentioned this pull request Jun 9, 2017

test,async_hooks: stabilize tests on Windows #13381

Merged

11 tasks

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

MylesBorins added the dont-land-on-v6.x label Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: increase coverage of async_hooks #13336

test: increase coverage of async_hooks #13336

DavidCai1111 commented May 31, 2017

kunalspathak commented May 31, 2017

addaleax May 31, 2017

DavidCai1111 Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

AndreasMadsen Jun 1, 2017

DavidCai1111 Jun 1, 2017

kunalspathak commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

refack commented Jun 1, 2017

refack commented Jun 1, 2017

DavidCai1111 commented Jun 2, 2017

AndreasMadsen left a comment

trevnorris left a comment

trevnorris Jun 2, 2017

DavidCai1111 Jun 2, 2017

trevnorris Jun 6, 2017

trevnorris Jun 2, 2017

DavidCai1111 Jun 2, 2017

DavidCai1111 Jun 5, 2017

trevnorris Jun 2, 2017

refack commented Jun 4, 2017

DavidCai1111 commented Jun 5, 2017

refack commented Jun 5, 2017

DavidCai1111 commented Jun 6, 2017

DavidCai1111 commented Jun 6, 2017 •

edited

Loading

DavidCai1111 commented Jun 6, 2017

trevnorris commented Jun 6, 2017

DavidCai1111 commented Jun 7, 2017

test: increase coverage of async_hooks #13336

test: increase coverage of async_hooks #13336

Conversation

DavidCai1111 commented May 31, 2017

Checklist

Affected core subsystem(s)

kunalspathak commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

refack commented Jun 1, 2017

refack commented Jun 1, 2017

DavidCai1111 commented Jun 2, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jun 4, 2017

DavidCai1111 commented Jun 5, 2017

refack commented Jun 5, 2017

DavidCai1111 commented Jun 6, 2017

DavidCai1111 commented Jun 6, 2017 • edited Loading

DavidCai1111 commented Jun 6, 2017

trevnorris commented Jun 6, 2017

DavidCai1111 commented Jun 7, 2017

DavidCai1111 commented Jun 6, 2017 •

edited

Loading