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

tests/net_test: don't try to resolve external names #1134

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

daniloegea
Copy link
Contributor

The test case "address_creation" relies on resolving the name www.example.com to succeed. It means that testing the project on environments without access to a DNS server will fail. Use the name "localhost" instead so the local resolver will not need to query a DNS server.

The test case "address_creation" relies on resolving the name
www.example.com to succeed. It means that testing the project on
environments without access to a DNS server will fail. Use the name
"localhost" instead so the local resolver will not need to query a DNS
server.
@Tachi107
Copy link
Member

Hi @daniloegea, thanks for your patch. While using localhost looks like it'd make sense, Pistache is currently hardcoded to return 127.0.0.1 if localhost is specified, meaning that dropping www.example.com would reduce test coverage. Does it make sense to you? Or would it be better to let getaddrinfo() handle localhost itself?

The net_test is special and opt-in because it requires internet access; how did you run into issues with it?

@daniloegea
Copy link
Contributor Author

Hi @Tachi107, thanks for replying.

I came across this issue while investigating why the Pistache Debian package was failing to build on Ubuntu. The reason is that the Ubuntu builders don't have internet access. And it's not rare to see CI pipelines with restricted network access.

In my opinion it would be a better approach to let the system's libc resolve names, even for localhost, otherwise the library won't follow whatever the user added to /etc/hosts. Or at least the call to the system's resolver should be mocked.

Besides, is it guaranteed that www.example.com will not start resolving to a different IP in the future?

@kiplingw
Copy link
Member

@daniloegea, in case it helps, unit tests that require network connectivity are disabled when the Debian package is being built.

@Tachi107
Copy link
Member

@daniloegea I do agree with you and I think that Pistache shouldn't be special casing localhost, but it should simply always use the system's resolver with getaddrinfo(). I'm going to submit a patch to do just that pretty soon.

By the way, what do you mean by building the Debian package on Ubuntu? Are you trying to package Pistache in Ubuntu's main repos or are you just trying to build it in a "third party" repository?

@daniloegea
Copy link
Contributor Author

@kiplingw thanks for the suggestion. That's interesting, Debian doesn't really use this rule: https://tracker.debian.org/media/packages/p/pistache/rules-0.0.5ds-3. And I just realized that @Tachi107 is the package's maintainer :)

@Tachi107 No, Ubuntu already has the package (https://launchpad.net/ubuntu/+source/pistache). Ubuntu basically copies the package from Debian as it is and rebuilds it. We probably hit this issue because debian/rules is not skipping the test suit that requires network as @kiplingw mentioned.

@Tachi107
Copy link
Member

That's interesting, Debian doesn't really use this rule:

Weird, I probably forgot to turn it on. I'll fix it as soon as possible

@Tachi107
Copy link
Member

Fixed in version 0.0.5+ds-4. It will soon hit the Debian archive

@kiplingw kiplingw merged commit e884d6d into pistacheio:master Jun 23, 2023
@Tachi107
Copy link
Member

Note: with this merged, tests doesn't cover the getaddrinfo() portion of the code. I intend to change that, but I didn't do it yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants