Skip to content
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

added test for dns/promise-onlookup.js #22897

Closed
wants to merge 4 commits into from

Conversation

mritunjayz
Copy link
Contributor

test for onlookup promise return of dns/promise
#22471

Checklist

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 17, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome @mritunjaygoutam12 and thanks for the pull request! Can you indicate what lines in lib/internal/dns/promises.js this covers that aren't already covered by tests in test/internet/test-dns-* and elsewhere? I think this may be best on outdated coverage reports (such as https://coverage.nodejs.org/coverage-9782ce25750f2a34/root/internal/dns/promises.js.html). More recent coverage reports (such as https://coverage.nodejs.org/coverage-988be43e3304be4a/root/internal/dns/promises.js.html) tell a different story (although it would be great to get those last few bits covered).

@mritunjayz
Copy link
Contributor Author

@Trott Thank you. Now recently I got how to see and understand coverage report. Or one more thing I get confused because there is no test file named for onlookup. I think it was covered by test-dns-lookup.:expressionless::expressionless::expressionless:

@Trott
Copy link
Member

Trott commented Sep 19, 2018

If I understand everything correctly, this duplicates existing test functionality so I'm going to close it. If I'm wrong about that, please comment!

@mritunjaygoutam12 Thanks for the pull request. I hope you will try again, perhaps referring to more current coverage stats. Thanks!

@Trott Trott closed this Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants