Skip to content

test: fix failing coverage adding tests on closeAllConnections #6083

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cecia234
Copy link
Contributor

I saw that the coverage threshold is not currently satisfied locally, since lines 172-173 in lib/server.js are not covered.

In #6069 this was preventing my commit since the pre-commit hook was failing locally. On CI pipeline it should not be a problem, it seems to work and the coverage is at 100%.

I added the tests to fix the issue.

Mocking dns.lookup is needed in order to ensure that the secondary server always gets created when host is set to localhost. Even when the all options is set to true the OS can choose to return only the IPv4 address, resulting in the missing creation of the secondary server.

Checklist

@cecia234 cecia234 force-pushed the test/close-all-connections branch 2 times, most recently from cf95331 to f7536c2 Compare April 27, 2025 16:07
@cecia234
Copy link
Contributor Author

Still need to understand why the test fails on node 20 and not on node 22.

@Eomm Eomm added the test Issue or pr related to our testing infrastructure. label Apr 30, 2025
@cecia234 cecia234 force-pushed the test/close-all-connections branch from 21cdd7f to bb4fe88 Compare May 11, 2025 13:33
@cecia234 cecia234 force-pushed the test/close-all-connections branch from bb4fe88 to 327e9a4 Compare May 11, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issue or pr related to our testing infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants