-
Notifications
You must be signed in to change notification settings - Fork 341
fix: Setting a default timeout on all HTTP connections #397
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,53 +28,43 @@ def test_http_client_default_session(): | |
client = _http_client.HttpClient() | ||
assert client.session is not None | ||
assert client.base_url == '' | ||
assert client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
recorder = _instrument(client, 'body') | ||
resp = client.request('get', _TEST_URL) | ||
assert resp.status_code == 200 | ||
assert resp.text == 'body' | ||
assert len(recorder) == 1 | ||
assert recorder[0].method == 'GET' | ||
assert recorder[0].url == _TEST_URL | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( | ||
_http_client.DEFAULT_TIMEOUT_SECONDS, 0.001) | ||
|
||
def test_http_client_custom_session(): | ||
session = requests.Session() | ||
client = _http_client.HttpClient(session=session) | ||
assert client.session is session | ||
assert client.base_url == '' | ||
assert client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
recorder = _instrument(client, 'body') | ||
resp = client.request('get', _TEST_URL) | ||
assert resp.status_code == 200 | ||
assert resp.text == 'body' | ||
assert len(recorder) == 1 | ||
assert recorder[0].method == 'GET' | ||
assert recorder[0].url == _TEST_URL | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( | ||
_http_client.DEFAULT_TIMEOUT_SECONDS, 0.001) | ||
|
||
def test_base_url(): | ||
client = _http_client.HttpClient(base_url=_TEST_URL) | ||
assert client.session is not None | ||
assert client.base_url == _TEST_URL | ||
assert client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
recorder = _instrument(client, 'body') | ||
resp = client.request('get', 'foo') | ||
assert resp.status_code == 200 | ||
assert resp.text == 'body' | ||
assert len(recorder) == 1 | ||
assert recorder[0].method == 'GET' | ||
assert recorder[0].url == _TEST_URL + 'foo' | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( | ||
_http_client.DEFAULT_TIMEOUT_SECONDS, 0.001) | ||
|
||
def test_credential(): | ||
client = _http_client.HttpClient( | ||
credential=testutils.MockGoogleCredential()) | ||
assert client.session is not None | ||
assert client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
recorder = _instrument(client, 'body') | ||
resp = client.request('get', _TEST_URL) | ||
assert resp.status_code == 200 | ||
|
@@ -83,26 +73,27 @@ def test_credential(): | |
assert recorder[0].method == 'GET' | ||
assert recorder[0].url == _TEST_URL | ||
assert recorder[0].headers['Authorization'] == 'Bearer mock-token' | ||
|
||
def test_default_timeout(): | ||
client = _http_client.HttpClient() | ||
assert client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
recorder = _instrument(client, 'body') | ||
client.request('get', _TEST_URL) | ||
assert len(recorder) == 1 | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx( | ||
_http_client.DEFAULT_TIMEOUT_SECONDS, 0.001) | ||
|
||
@pytest.mark.parametrize('timeout', [7, None]) | ||
def test_timeout(timeout): | ||
@pytest.mark.parametrize('timeout', [7, 0, None]) | ||
def test_custom_timeout(timeout): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function looks mostly identical to test_default_timeout. You could optionally refactor to something like this: def _helper_test_timeout(**kwargs):
client = _http_client.HttpClient()
timeout = kwargs.get('timeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
assert client.timeout == timeout
...
if timeout is None
assert recorder[0]._extra_kwargs['timeout'] is None
else:
assert ... == pytest.approx(timeout, 0.001)
def test_default_timeout():
_helper_test_timeout()
@parameterize(...)
def test_custom_timeout(timeout)
_helper_test_timeout(timeout=timeout) (Or you could parameterize the helper directly.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactored with parameterization. |
||
client = _http_client.HttpClient(timeout=timeout) | ||
assert client.session is not None | ||
assert client.base_url == '' | ||
assert client.timeout == timeout | ||
recorder = _instrument(client, 'body') | ||
resp = client.request('get', _TEST_URL) | ||
assert resp.status_code == 200 | ||
assert resp.text == 'body' | ||
client.request('get', _TEST_URL) | ||
assert len(recorder) == 1 | ||
assert recorder[0].method == 'GET' | ||
assert recorder[0].url == _TEST_URL | ||
if timeout: | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001) | ||
else: | ||
if timeout is None: | ||
assert recorder[0]._extra_kwargs['timeout'] is None | ||
else: | ||
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(timeout, 0.001) | ||
|
||
|
||
def _instrument(client, payload, status=200): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -523,14 +523,33 @@ def _assert_request_is_correct( | |
assert request.url == expected_url | ||
client_version = 'Python/Admin/{0}'.format(firebase_admin.__version__) | ||
assert request.headers['X-Client-Version'] == client_version | ||
assert request._extra_kwargs['timeout'] == pytest.approx( | ||
_http_client.DEFAULT_TIMEOUT_SECONDS, 0.001) | ||
if expected_body is None: | ||
assert request.body is None | ||
else: | ||
assert json.loads(request.body.decode()) == expected_body | ||
|
||
|
||
class TestTimeout(BaseProjectManagementTest): | ||
|
||
def test_default_timeout(self): | ||
app = firebase_admin.get_app() | ||
project_management_service = project_management._get_project_management_service(app) | ||
assert project_management_service._client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS | ||
|
||
@pytest.mark.parametrize('timeout', [4, None]) | ||
def test_custom_timeout(self, timeout): | ||
options = { | ||
'httpTimeout': timeout, | ||
'projectId': 'test-project-id' | ||
} | ||
app = firebase_admin.initialize_app(testutils.MockCredential(), options, 'timeout-app') | ||
try: | ||
project_management_service = project_management._get_project_management_service(app) | ||
assert project_management_service._client.timeout == timeout | ||
finally: | ||
firebase_admin.delete_app(app) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BaseProjectManagementTest seems to call testutils.cleanup_apps() in it's teardown() method. If you're willing to rely on that, you could drop the try, finally, and delete_app() lines. Presumably you'd nave to not hard-code the app name ("timeout-app") but instead use something based on the test parameter or something like that instead. Optional. I have no objection to leaving it in either, but it's odd to see the app cleaned up in this test case, but not the one above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
|
||
class TestCreateAndroidApp(BaseProjectManagementTest): | ||
_CREATION_URL = 'https://firebase.googleapis.com/v1beta1/projects/test-project-id/androidApps' | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: the 'which is the default behaviour ...' part is no longer true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is still true. The
requests
library behavior hasn't yet changed. The defaulttimeout=None
behavior still comes from there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops; yeah I missed the change from 'if unset' to 'if set to none' (despite suggesting exactly that earlier. 😳)