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

fixed network test #8498

Merged
merged 1 commit into from
Dec 4, 2020
Merged

fixed network test #8498

merged 1 commit into from
Dec 4, 2020

Conversation

helenosheaa
Copy link
Member

Fixed network test by adding a mock client for failing test coming back with connection failure instead of dns failure

@srebhan srebhan added the fix pr to fix corresponding bug label Dec 4, 2020
@srebhan
Copy link
Member

srebhan commented Dec 4, 2020

The problem only occurs if you set a proxy in your environment. Unsetting it will make the test pass. The reason behind is that you get a "Bad gateway" error message from your proxy (Squid at least) instead of the DNS error expected.
So I'd rather like to ask you to implement a way for the plugin to ignore the environment proxy, e.g. by setting the http_proxy option to "-". This is helpful in production environments and allows us to force the tests into not using any proxy a.k.a. reproducible environment. :-)

Are you willing to do this?

@srebhan srebhan self-assigned this Dec 4, 2020
@ssoroka
Copy link
Contributor

ssoroka commented Dec 4, 2020

@srebhan I think it's a bigger issue that this test relies on DNS failure from a live request. There are lots of ways for internet-destined requests to fail, and I generally don't like tests requiring internet access. stubbing out the response accomplishes what the test intended.

@helenosheaa helenosheaa merged commit ef6ce2c into master Dec 4, 2020
@helenosheaa helenosheaa deleted the http-response-network-test branch December 4, 2020 17:08
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants