test: add a test for Server. listen callback runtime binding#35657
test: add a test for Server. listen callback runtime binding#35657adinhtdsibm wants to merge 4 commits intonodejs:masterfrom adinhtdsibm:opensnodejsupdate
Conversation
modified the file test-http-pause.js by replacing the function ()=>
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Hello 👋 Thanks for your work, and welcome to the Node.js repo.
I think using an anonymous function for this test was on purpose, because we want to make sure the callback is bind to the server instance. By replacing it with an arrow function, this PR removes an important test.
What I suggest instead is to make the use of an anonymous function explicit with a comment and an equality check.
Updating the changes as requested above. Co-authored-by: Antoine du Hamel <[email protected]>
Updated the changes as requested. Co-authored-by: Antoine du Hamel <[email protected]>
Committed the changes as requested. Thanks for the information provided for the updates. |
aduh95
added
author ready
github-actions
bot
removed
the
request-ci
This comment has been minimized.
This comment has been minimized.
|
Note to who ever merges this PR: the original commit message needs to change to reflect what this PR is now doing; maybe something like: |
adinhtdsibm
changed the title
PR-URL: #35657 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
|
landed in 9ce5a03 . Thanks for the contribution! |
PR-URL: #35657 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #35657 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes