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

dns.promises test coverage is poor #22471

Closed
ChALkeR opened this issue Aug 23, 2018 · 5 comments
Closed

dns.promises test coverage is poor #22471

ChALkeR opened this issue Aug 23, 2018 · 5 comments
Labels
dns Issues and PRs related to the dns subsystem. experimental Issues and PRs related to experimental features. promises Issues and PRs related to ECMAScript promises. test Issues and PRs related to the tests.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Aug 23, 2018

Ref: #20435 (comment)

Experimental dns/promises API is present in the current master without proper test coverage.

According to the coverage report, the following functions are not tested at all:

  • onlookup
  • onlookupall
  • onlookupservice
  • createLookupServicePromise

While inspecting the experimental fs.promises, there were also some implementation bugs found that could have been caught by code coverage, see e.g. #20407. The dns.promises is much smaller in lines of code, but it is still possible that it contains bugs that could be noticed by extending coverage.

@ChALkeR ChALkeR added dns Issues and PRs related to the dns subsystem. promises Issues and PRs related to ECMAScript promises. experimental Issues and PRs related to experimental features. test Issues and PRs related to the tests. labels Aug 23, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 23, 2018

#21559 should partially (?) fix this, I presume.

@BridgeAR BridgeAR added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Aug 23, 2018
@SirR4T
Copy link
Contributor

SirR4T commented Aug 24, 2018

Willing to work towards this, with some mentoring/help. For example, would a common.dnsTest() be helpful here, similar to common.fsTest() from #20439?

@sagitsofan
Copy link
Contributor

sagitsofan commented Sep 10, 2018

@ChALkeR I will be happy to contribute in this issue.
Is there is an existing test file or should i create a new one (and if so - in which libary under "test"?)

@Trott
Copy link
Member

Trott commented Sep 11, 2018

It's considerably less poor now that we run internet tests during coverage report generation. Right now, it's at 64 of 67 branches. Prior to that, it was 40 of 67.

I'd welcome someone getting those last 3 branches covered, but I think this can nonetheless be closed at this time.

@Trott Trott removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Sep 18, 2018
@Trott
Copy link
Member

Trott commented Sep 18, 2018

I'm going to close this. Feel free to re-open if you think that's not warranted at this time, @ChALkeR.

@Trott Trott closed this as completed Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem. experimental Issues and PRs related to experimental features. promises Issues and PRs related to ECMAScript promises. test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

5 participants