-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: add tests for dnsPromises.lookup #21559
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.
There are already tests for these things in the internet
suite. Unfortunately, those don't count toward code coverage.
@cjihrig Code coverage report for internal/dns/promises.js( |
@cjihrig I found the tests for these in |
Generally, tests that require the Internet should be in the |
Thanks for your reply. OK, I'll fix this. |
@cjihrig The tests that require the Internet were removed. But tests to check error with invalid host name are remained to improve coverage. Please take a look again. |
Added tests for dnsPromises.lookup to increase coverage and test `onlookup()` and `onlookupall()` methods.
490df72
to
116f6eb
Compare
@jasnell node-test-commit-freebsd fails but I think it is not related to this. I'm not sure if it will be success, but I rebased this. Would you please run CI again? |
freebsd re-build: https://ci.nodejs.org/job/node-test-commit-freebsd/19630/ |
@addaleax did a wonderful job of writing a mock DNS server in
|
Landed in 2118342 |
Added tests for dnsPromises.lookup to increase coverage and test `onlookup()` and `onlookupall()` methods. PR-URL: #21559 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Added tests for dnsPromises.lookup to increase coverage and test `onlookup()` and `onlookupall()` methods. PR-URL: #21559 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
The remaining test cases still rely on internet access and will fail if the DNS is hijacked by the ISP. I move them to internet in #22516 |
Added tests for dnsPromises.lookup to increase coverage and test `onlookup()` and `onlookupall()` methods. PR-URL: #21559 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
|
Added tests for dnsPromises.lookup to increase coverage and test
onlookup()
andonlookupall()
methods.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes