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

Unify timeouts in tests #1717

Merged
merged 6 commits into from Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions test/__init__.py
Expand Up @@ -28,6 +28,16 @@
# RFC 3849: 2001:db8::/32 is for documentation only.
INVALID_SOURCE_ADDRESSES = [("192.0.2.255", 0), ("2001:db8::1", 0)]

# We use timeouts in three different ways in our tests
#
# 1. To make sure that the operation timeouts, we can use a short timeout.
# 2. To make sure that the test does not hang even if the operation should succeed, we
# want to use a long timeout, even more so on CI where tests can be really slow
# 3. To test our timeout logic by using two different values, eg. by using different
# values at the pool level and at the request level.
SHORT_TIMEOUT = 0.001
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Travis (which is faster than AppVeyor usually) timed out on this 0.001 value. Maybe we need to have two separate values for the SHORT_TIMEOUT as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, test_ssl_read_timeout failed, I had not noticed that, sorry. I think the better fix is to switch to LONG_TIMEOUT here. Here's my reasoning.

In this test, we:

  1. read the headers -- this should not timeout, so using LONG_TIMEOUT makes sense
  2. read the body -- this should timeout, so using SHORT_TIMEOUT makes sense

However, both timeouts are read timeouts and are controlled by a single value, so we have to compromise. Switching to LONG_TIMEOUT is only a compromise for this test, while bumping SHORT_TIMEOUT would be a compromise for all tests.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LONG_TIMEOUT works for that test.

LONG_TIMEOUT = 0.5 if os.environ.get("CI") else 0.01


def clear_warnings(cls=HTTPWarning):
new_filters = []
Expand Down
7 changes: 6 additions & 1 deletion test/appengine/test_gae_manager.py
Expand Up @@ -7,6 +7,7 @@
import urllib3.util.retry

from test.with_dummyserver import test_connectionpool
from test import SHORT_TIMEOUT


# This class is used so we can re-use the tests from the connection pool.
Expand Down Expand Up @@ -46,7 +47,11 @@ def setup_method(self, method):
def test_exceptions(self):
# DeadlineExceededError -> TimeoutError
with pytest.raises(urllib3.exceptions.TimeoutError):
self.pool.request("GET", "/sleep?seconds=0.005", timeout=0.001)
self.pool.request(
"GET",
"/sleep?seconds={}".format(5 * SHORT_TIMEOUT),
timeout=SHORT_TIMEOUT,
)

# InvalidURLError -> ProtocolError
with pytest.raises(urllib3.exceptions.ProtocolError):
Expand Down
5 changes: 4 additions & 1 deletion test/contrib/test_socks.py
Expand Up @@ -8,6 +8,7 @@
from dummyserver.testcase import IPV4SocketDummyServerTestCase

import pytest
from test import SHORT_TIMEOUT

try:
import ssl
Expand Down Expand Up @@ -314,7 +315,9 @@ def request_handler(listener):
proxy_url = "socks5h://%s:%s" % (self.host, self.port)
with socks.SOCKSProxyManager(proxy_url) as pm:
with pytest.raises(ConnectTimeoutError):
pm.request("GET", "http://example.com", timeout=0.001, retries=False)
pm.request(
"GET", "http://example.com", timeout=SHORT_TIMEOUT, retries=False
)
event.set()

def test_connection_failure(self):
Expand Down
15 changes: 8 additions & 7 deletions test/test_connectionpool.py
Expand Up @@ -31,6 +31,7 @@
from ssl import SSLError as BaseSSLError

from dummyserver.server import DEFAULT_CA
from test import SHORT_TIMEOUT


class HTTPUnixConnection(HTTPConnection):
Expand Down Expand Up @@ -212,13 +213,13 @@ def test_not_same_host_custom_protocol(self, a, b):

def test_max_connections(self):
with HTTPConnectionPool(host="localhost", maxsize=1, block=True) as pool:
pool._get_conn(timeout=0.01)
pool._get_conn(timeout=SHORT_TIMEOUT)

with pytest.raises(EmptyPoolError):
pool._get_conn(timeout=0.01)
pool._get_conn(timeout=SHORT_TIMEOUT)

with pytest.raises(EmptyPoolError):
pool.request("GET", "/", pool_timeout=0.01)
pool.request("GET", "/", pool_timeout=SHORT_TIMEOUT)

assert pool.num_connections == 1

Expand Down Expand Up @@ -288,7 +289,7 @@ def _test(exception, expect, reason=None):
# See: https://github.com/shazow/urllib3/issues/76
pool._make_request = lambda *args, **kwargs: _raise(HTTPException)
with pytest.raises(MaxRetryError):
pool.request("GET", "/", retries=1, pool_timeout=0.01)
pool.request("GET", "/", retries=1, pool_timeout=SHORT_TIMEOUT)
assert pool.pool.qsize() == POOL_SIZE

def test_assert_same_host(self):
Expand Down Expand Up @@ -348,9 +349,9 @@ def test_pool_timeouts(self):
assert pool.timeout._connect == Timeout.DEFAULT_TIMEOUT
assert pool.timeout.total is None

pool = HTTPConnectionPool(host="localhost", timeout=3)
assert pool.timeout._read == 3
assert pool.timeout._connect == 3
pool = HTTPConnectionPool(host="localhost", timeout=SHORT_TIMEOUT)
assert pool.timeout._read == SHORT_TIMEOUT
assert pool.timeout._connect == SHORT_TIMEOUT
assert pool.timeout.total is None

def test_no_host(self):
Expand Down
14 changes: 4 additions & 10 deletions test/with_dummyserver/test_connectionpool.py
Expand Up @@ -25,6 +25,7 @@
from urllib3.util.retry import Retry, RequestHistory
from urllib3.util.timeout import Timeout

from test import SHORT_TIMEOUT, LONG_TIMEOUT
from dummyserver.testcase import HTTPDummyServerTestCase, SocketDummyServerTestCase
from dummyserver.server import NoIPv6Warning, HAS_IPV6_AND_DNS

Expand All @@ -37,10 +38,6 @@
log.addHandler(logging.StreamHandler(sys.stdout))


SHORT_TIMEOUT = 0.001
LONG_TIMEOUT = 0.03


def wait_for_socket(ready_event):
ready_event.wait()
ready_event.clear()
Expand All @@ -51,19 +48,16 @@ def test_timeout_float(self):
block_event = Event()
ready_event = self.start_basic_handler(block_send=block_event, num=2)

# Pool-global timeout
with HTTPConnectionPool(
self.host, self.port, timeout=SHORT_TIMEOUT, retries=False
) as pool:
with HTTPConnectionPool(self.host, self.port, retries=False) as pool:
wait_for_socket(ready_event)
with pytest.raises(ReadTimeoutError):
pool.request("GET", "/")
pool.request("GET", "/", timeout=SHORT_TIMEOUT)
block_event.set() # Release block

# Shouldn't raise this time
wait_for_socket(ready_event)
block_event.set() # Pre-release block
pool.request("GET", "/")
pool.request("GET", "/", timeout=LONG_TIMEOUT)

def test_conn_closed(self):
block_event = Event()
Expand Down
12 changes: 7 additions & 5 deletions test/with_dummyserver/test_https.py
Expand Up @@ -39,6 +39,8 @@
requiresTLSv1_2,
requiresTLSv1_3,
TARPIT_HOST,
SHORT_TIMEOUT,
LONG_TIMEOUT,
)
from urllib3 import HTTPSConnectionPool
from urllib3.connection import VerifiedHTTPSConnection, RECENT_DATE
Expand Down Expand Up @@ -503,7 +505,7 @@ def test_good_fingerprint_and_hostname_mismatch(self):
@requires_network
def test_https_timeout(self):

timeout = Timeout(total=None, connect=0.001)
timeout = Timeout(total=None, connect=SHORT_TIMEOUT)
with HTTPSConnectionPool(
TARPIT_HOST,
self.port,
Expand Down Expand Up @@ -553,7 +555,7 @@ def test_enhanced_timeout(self):
with HTTPSConnectionPool(
TARPIT_HOST,
self.port,
timeout=Timeout(connect=0.001),
timeout=Timeout(connect=SHORT_TIMEOUT),
retries=False,
cert_reqs="CERT_REQUIRED",
) as https_pool:
Expand All @@ -569,12 +571,12 @@ def test_enhanced_timeout(self):
with HTTPSConnectionPool(
TARPIT_HOST,
self.port,
timeout=Timeout(connect=5),
timeout=Timeout(connect=LONG_TIMEOUT),
retries=False,
cert_reqs="CERT_REQUIRED",
) as https_pool:
with pytest.raises(ConnectTimeoutError):
https_pool.request("GET", "/", timeout=Timeout(connect=0.001))
https_pool.request("GET", "/", timeout=Timeout(connect=SHORT_TIMEOUT))

with HTTPSConnectionPool(
TARPIT_HOST,
Expand All @@ -587,7 +589,7 @@ def test_enhanced_timeout(self):
try:
with pytest.raises(ConnectTimeoutError):
https_pool.request(
"GET", "/", timeout=Timeout(total=None, connect=0.001)
"GET", "/", timeout=Timeout(total=None, connect=SHORT_TIMEOUT)
)
finally:
conn.close()
Expand Down
6 changes: 4 additions & 2 deletions test/with_dummyserver/test_poolmanager.py
Expand Up @@ -9,6 +9,8 @@
from urllib3.exceptions import MaxRetryError
from urllib3.util.retry import Retry

from test import LONG_TIMEOUT

# Retry failed tests
pytestmark = pytest.mark.flaky

Expand Down Expand Up @@ -88,15 +90,15 @@ def test_cross_host_redirect(self):
"GET",
"%s/redirect" % self.base_url,
fields={"target": cross_host_location},
timeout=1,
timeout=LONG_TIMEOUT,
retries=0,
)

r = http.request(
"GET",
"%s/redirect" % self.base_url,
fields={"target": "%s/echo?a=b" % self.base_url_alt},
timeout=1,
timeout=LONG_TIMEOUT,
retries=1,
)

Expand Down
16 changes: 9 additions & 7 deletions test/with_dummyserver/test_proxy_poolmanager.py
Expand Up @@ -12,6 +12,8 @@
from urllib3.exceptions import MaxRetryError, SSLError, ProxyError, ConnectTimeoutError
from urllib3.connectionpool import connection_from_url, VerifiedHTTPSConnection

from test import SHORT_TIMEOUT, LONG_TIMEOUT

# Retry failed tests
pytestmark = pytest.mark.flaky

Expand Down Expand Up @@ -54,7 +56,7 @@ def test_nagle_proxy(self):
def test_proxy_conn_fail(self):
host, port = get_unreachable_address()
with proxy_from_url(
"http://%s:%s/" % (host, port), retries=1, timeout=0.05
"http://%s:%s/" % (host, port), retries=1, timeout=LONG_TIMEOUT
) as http:
with pytest.raises(MaxRetryError):
http.request("GET", "%s/" % self.https_url)
Expand Down Expand Up @@ -134,15 +136,15 @@ def test_cross_host_redirect(self):
"GET",
"%s/redirect" % self.http_url,
fields={"target": cross_host_location},
timeout=1,
timeout=LONG_TIMEOUT,
retries=0,
)

r = http.request(
"GET",
"%s/redirect" % self.http_url,
fields={"target": "%s/echo?a=b" % self.http_url_alt},
timeout=1,
timeout=LONG_TIMEOUT,
retries=1,
)
assert r._pool.host != self.http_host_alt
Expand All @@ -155,15 +157,15 @@ def test_cross_protocol_redirect(self):
"GET",
"%s/redirect" % self.http_url,
fields={"target": cross_protocol_location},
timeout=1,
timeout=LONG_TIMEOUT,
retries=0,
)

r = http.request(
"GET",
"%s/redirect" % self.http_url,
fields={"target": "%s/echo?a=b" % self.https_url},
timeout=1,
timeout=LONG_TIMEOUT,
retries=1,
)
assert r._pool.host == self.https_host
Expand Down Expand Up @@ -321,14 +323,14 @@ def test_proxy_pooling_ext(self):
def test_https_proxy_timeout(self):
with proxy_from_url("https://{host}".format(host=TARPIT_HOST)) as https:
with pytest.raises(MaxRetryError) as e:
https.request("GET", self.http_url, timeout=0.001)
https.request("GET", self.http_url, timeout=SHORT_TIMEOUT)
assert type(e.value.reason) == ConnectTimeoutError

@pytest.mark.timeout(0.5)
@requires_network
def test_https_proxy_pool_timeout(self):
with proxy_from_url(
"https://{host}".format(host=TARPIT_HOST), timeout=0.001
"https://{host}".format(host=TARPIT_HOST), timeout=SHORT_TIMEOUT
) as https:
with pytest.raises(MaxRetryError) as e:
https.request("GET", self.http_url)
Expand Down