-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
test: improve dns lookup coverage #30777
Conversation
@nodejs/dns @nodejs/testing |
@KeeReal Welcome and thanks for the pull request. Are you able to point to the lines of code that are currently uncovered that this will cover? Or at least what file those lines of code would be in? |
Hi @Trott. |
@joyeecheung (just in case you want to test on any particularly restrictive networks--I tested with the network disconnected entirely and the test worked fine, which I suppose is a good indication that the monkey-patching has the intended effect, but just in case....) |
@KeeReal were you able to run coverage locally to make sure that you hit the target lines: NODE_V8_COVERAGE=./cov ./node test/parallel/test-dns-lookup-promises.js
./node_modules/.bin/c8 report --reporter=html \
--temp-directory=./cov --omit-relative=false \
--resolve=./lib --exclude="benchmark/" --exclude="deps/" --exclude="test/" --exclude="tools/" \
--wrapper-length=0
open coverage/index.html |
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 looks good to me, conditional on confirming that this test exercises the expected code.
Thx, @bcoe |
Adding tests covering promises-related code paths.
beb6e46
to
494f32d
Compare
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.
RSLGTM
Mocking is definitely not ideal but this part can probably not be tested properly otherwise.
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/27587/ ✅ (yellow build with two typical Windows flakes) |
Adding tests covering promises-related code paths. PR-URL: #30777 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Landed in 2d13896 🎉 |
Adding tests covering promises-related code paths. PR-URL: #30777 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adding tests covering promises-related code paths. PR-URL: #30777 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adding tests covering promises-related code paths. PR-URL: #30777 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Adding tests covering promises-related code paths.
missing coverage
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes