Ensure session timeout is used, add default #68
Conversation
There was a problem hiding this comment.
However, for now there is no way to actually provide a timeout value to the requests function without hacking the code.
I’m not sure I’m following. Do you have an example of what you mean? It seems like someone could still do:
session = wayback.WaybackSession()
# Custom timeout:
session.request('http://something.com/', timeout=5)
# or no timeout:
session.request('http://something.com/', timeout=None)
|
Also, could you please add tests for this? |
Sure. I've not yet worked with pytest so give me a couple days. |
|
So, I've added the the tests. I've forgotten to add the mock library to the dev dependencies, that is what the third commit was for. |
wayback/tests/test_client.py
Outdated
| @@ -2,6 +2,8 @@ | |||
| from pathlib import Path | |||
| import pytest | |||
| import vcr | |||
| import requests | |||
| import mock | |||
There was a problem hiding this comment.
Since this project requires Python 3.6+, you can just use the built-in unittest.mock library rather than adding a new dependency. mock is a backport of the built-in for older versions of Python.
There was a problem hiding this comment.
Huh, I didn't know that mock came build in with unittest.
I've changed that (and the typos below).
wayback/tests/test_client.py
Outdated
| @@ -474,6 +476,14 @@ def test_get_memento_follow_redirects_does_not_follow_historical_redirects(): | |||
| assert len(memento.debug_history) == 1 | |||
|
|
|||
|
|
|||
| def return_timeout(self, *args, **kwargs) -> requests.Response: | |||
| """Overwirte the requests.Session.send to return a response with the provied timeout value as content""" | |||
There was a problem hiding this comment.
Typo:
| """Overwirte the requests.Session.send to return a response with the provied timeout value as content""" | |
| """Overwrite requests.Session.send to return a response with the provided timeout value as content.""" |
| @mock.patch('requests.Session.send', side_effect=return_timeout) | ||
| def test_timeout_applied_session(self, mock_class): | ||
| # Is the timeout applied through the WaybackSession | ||
| session = WaybackSession(timeout=1) | ||
| res = session.request('GET', 'http://test.com') | ||
| assert res.text == '1' | ||
| # Overwriting the default in the requests method | ||
| res = session.request('GET', 'http://test.com', timeout=None) | ||
| assert res.text == 'None' | ||
| res = session.request('GET', 'http://test.com', timeout=2) | ||
| assert res.text == '2' |
There was a problem hiding this comment.
This is hacky, but I think it works fine for now! I’m fine merging this. 👍
If you want to do something fancier, you could check out this timeout test in web-monitoring-diff: https://github.com/edgi-govdata-archiving/web-monitoring-diff/blob/2fe3088b04bb2a58fdb5fec1e92d4fed181233f3/web_monitoring_diff/tests/test_server_exc_handling.py#L192-L202
(It tests that a proxy server correctly times out while contacting the “upstream” server with the actual content. The patch decorator makes the proxy being tested have a short timeout, and responder and SimpleHttpServer create a quick little HTTP server to use as the upstream, which waits longer than the expected timeout before responding. That’s way more complex than what you’re doing here, though. It would be more complete, but not really necessary. Also note that package uses async/await, so the solution here couldn’t be exactly the same.)
|
Is this good to go? If so, I’ll merge later tonight and backport to the v0.2.x release line. (And then publish v0.2.6.) |
|
From my side it is good to go. |
|
@LionSzl I should have asked this before: would you like to be added to the contributors list in the README? |
In #68, we forgot to update the documentation to specify the new default value.
Sure, if one bugfix qualifies me as contributor... My full name is Lion Szlagowski. |
|
Of course it does! And you are now in the README. 😄 |
|
I’ve also backported the bugfix part of this and published it as v0.2.6. |
The bug described in #66, therefore the session now respects the default ratelimit, if no other ratelimit was provided when calling
WaybackSession.request.Default timeout of 60 seconds.