test: revise test-http-client-default-headers-exist #32493
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| }, expectedMethods.length)); | ||
|
|
||
| server.listen(0, common.mustCall(() => { | ||
| expectedMethods.forEach((method) => { | ||
| http.request({ | ||
| Promise.all(expectedMethods.map(async (method) => { |
There was a problem hiding this comment.
It's not an issue with this particular test, but keep in mind that changing all of these to use Promises is changing the timing of these tests... rather than executing the countdown cleanup logic synchronously it is being deferred to the microtask queue. That could end up causing the test to miss issues that may exist when closing things down synchronously so worth watching... also, keep this issue in mind also when refactoring these :-) .. as I said, not an issue on this particular test but there are some where it may be relevant.
There was a problem hiding this comment.
I want to make sure I understand your point about the cleanup code and the microtask queue. Are you saying that with countdown, the callback that checks things executes synchronously whenever the pertinent .dec() call happens, whereas with Promise.all(), it is whenever the microtask queue runs the resolve callback in the .then() call, so it might execute at a different time and with things in a different state than countdown?
If so: That's something I hadn't considered and worth watching out for. Thanks for pointing it out! (And thanks for making me take a few minutes to re-learn about the microtask queue, which is something I almost never think about!)
Trott
added
the
author ready
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/30132/ (:yellow_heart:) |
* Remove assert.strictEqual where assert.ok suffices * Replace countdown with Promise.all() PR-URL: nodejs#32493 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
|
Landed in 436c71d |
* Remove assert.strictEqual where assert.ok suffices * Replace countdown with Promise.all() PR-URL: #32493 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
* Remove assert.strictEqual where assert.ok suffices * Replace countdown with Promise.all() PR-URL: #32493 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes