-
Notifications
You must be signed in to change notification settings - Fork 29k
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
async_hooks,test: add test for async hooks parity for async/await #20626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work! I've left some notes!
test/async-hooks/test-async-await.js
Outdated
'after hook called for promise without prior call' + | ||
'to before hook'); | ||
promiseCallbacks.set(asyncId, 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove things from the map here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check if this still applies to the new version - now the maps are more meant to reflect the state of the promises created, so IMO no need to clean them in the hooks.
test/async-hooks/test-async-await.js
Outdated
const util = require('util'); | ||
|
||
const sleep = util.promisify(setTimeout); | ||
const promiseCallbacks = new Map(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are not really callbacks
, or maybe I got it wrong. Can we use a more clear name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed, thanks.
test/async-hooks/test-async-await.js
Outdated
assert.strictEqual(promiseCallbacks.get(asyncId), 1, | ||
'after hook called for promise without prior call' + | ||
'to before hook'); | ||
promiseCallbacks.set(asyncId, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we used something different from 0. Maybe can we use a string instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/async-hooks/test-async-await.js
Outdated
function checkPromiseCallbacks() { | ||
for (const balance of promiseCallbacks.values()) { | ||
assert.strictEqual(balance, 0, | ||
'mismatch between before and after hook calls'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would not verify if before and init fired. I know that these events would not fire for each promise, but maybe we should verify that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked the test. The only thing currently missing is the check for double calls of 'promiseResolve' hook, which, for some reason, fails in Node.js 8 and 10.
I'm not a huge fan of the test abstraction myself, but considering we are using it in every other test this should properly use:
for the validation. |
Add a basic test ensuring parity between before-after and init-promiseResolve hooks when using async/await. Refs: nodejs#20516
Add ability to initHooks and to checkInvocations utilities to transmit promiseResolve hook as well. Refs: nodejs#20516
Reword async/await test to make use of the initHooks utility and an attempt to clarify the test's logic. Refs: nodejs#20516
0475116
to
dc98e80
Compare
Regarding the previous run of the CI, I looked through the Jenkins output and both failing jobs have this message: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
test/async-hooks/test-async-await.js
Outdated
checkPromisesExecutionState(); | ||
})); | ||
|
||
async function asyncFunc(callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused callback
param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I think @TimothyGu comment has been addressed, landing now. |
Landed in e993e45 |
Add a basic test ensuring parity between before-after and init-promiseResolve hooks when using async/await. Add ability to initHooks and to checkInvocations utilities to transmit promiseResolve hook as well. See: #20516 PR-URL: #20626 Refs: #20516 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Congrats for your first PR to Node.js @MayaLekova! |
Add a basic test ensuring parity between before-after and init-promiseResolve hooks when using async/await. Add ability to initHooks and to checkInvocations utilities to transmit promiseResolve hook as well. See: #20516 PR-URL: #20626 Refs: #20516 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Add a basic test ensuring parity between before-after and init-promiseResolve hooks when using async/await. Add ability to initHooks and to checkInvocations utilities to transmit promiseResolve hook as well. See: nodejs#20516 PR-URL: nodejs#20626 Refs: nodejs#20516 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Add a basic test ensuring parity between before-after and
init-promiseResolve hooks when using async/await.
Refs: #20516
This is the first of a series of short commits adding a few conformance tests for async hooks. Using them will hopefully reduce the possibility of having regressions when we start to re-optimize the promises in V8. There will be separate tests in V8 as well.
Please feel free to give ideas about specific tests to include (the CLS use case is already planned) and if you prefer that I squash all the upcoming tests in a single commit or PR.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes