pistacheio / pistache

A high-performance REST toolkit written in C++
https://pistacheio.github.io/pistache/
Apache License 2.0
3.12k stars 688 forks source link

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

Closed daniloegea closed 1 year ago

daniloegea commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Tachi107 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

Tachi107 commented 1 year ago

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.