Support the use of a `Session` object #14

oliverlockwood · 2017-11-09T09:31:42Z

Closes #13.

A couple of things are worth highlighting up-front - I'll comment on these inline.

…re meaningful Session testing

…ession value

oliverlockwood · 2017-11-09T09:32:33Z

httsleep/main.py

@@ -12,6 +12,7 @@
 DEFAULT_POLLING_INTERVAL = 2 # in seconds
 DEFAULT_MAX_RETRIES = 50
 VALID_CONDITIONS = ['status_code', 'json', 'jsonpath', 'text', 'callback']
+DEFAULT_SESSION = requests.Session()


Since our PreparedRequest now comes from self.session.prepare_request(self.request) instead of self.request.prepare() - see below - there is a behavioural change in the default case (no Session explicitly provided).

From the links above, you can see that this involves merging in Session default headers, cookies, params, auth, and hooks. Of these, params and auth default to empty / none; I think the only interesting possibilities are around cookies and headers.

If we consider the behavioural change acceptable, then we're good. If on the other hand we want to ensure no behavioural change, we can change the definition of DEFAULT_SESSION to override cookies and headers to empty dictionaries. Your call.

Additionally (Python style question) should DEFAULT_SESSION be a method rather than an instance variable? I worry that the lack of immutability means that somebody could accidentally change DEFAULT_SESSION and unintentionally affect other uses of it.

coveralls · 2017-11-09T09:33:24Z

Coverage increased (+0.1%) to 95.313% when pulling 65ec4a4 on oliverlockwood:support-session into 1a5e76d on kopf:master.

coveralls · 2017-11-09T09:33:24Z

Coverage increased (+0.1%) to 95.313% when pulling 65ec4a4 on oliverlockwood:support-session into 1a5e76d on kopf:master.

coveralls · 2017-11-09T09:33:24Z

Coverage increased (+0.1%) to 95.313% when pulling 65ec4a4 on oliverlockwood:support-session into 1a5e76d on kopf:master.

oliverlockwood · 2017-11-09T09:41:25Z

tests/test_run.py

+        args, kwargs = mock_adapter_send.call_args
+    assert args[0].headers == {'Content-Type': 'test/type', 'Cookie': 'tasty-cookie=chocolate'}
+    assert 'verify' in kwargs
+    assert kwargs['verify'] == '/session/verify'


Initially, this test continued to fail even after I'd implemented the functionality.

The test case is where the session includes a specific value for verify, but the user doesn't directly specify a value in the HttSleeper API. Unfortunately, the session value was being ignored, and verify=True was being used instead, running counter to the standard behaviour of the requests library.

Thus I was forced to revisit the discussion we had in #12 (comment) about whether HttSleeper should by default send verify=True or leave the kwarg unspecified. That's why my 4th commit changes this behaviour back to that which I originally proposed in the last PR 😃

Aha, ok! Sorry for the back-and-forth, but at least the end result sounds perfect.

oliverlockwood · 2017-11-17T08:53:32Z

Hi @kopf, any chance of a review of this one when you have the chance? Many thanks.

kopf · 2017-11-17T09:22:25Z

yup, it's on my todo list, extremely stressful time at work at the moment, will have a look at the weekend

oliverlockwood · 2017-11-17T09:48:42Z

No worries!

kopf · 2017-11-19T10:14:40Z

tests/test_run.py

+    session = requests.Session()
+    session.cookies = {'tasty-cookie': 'chocolate'}
+    session.headers = {'Content-Type': 'test/type'}
+    session.verify = '/session/verify'


Should be a boolean, right?

Can be either a boolean or a string also - see https://github.com/requests/requests/blob/master/requests/api.py#L39-L41.

I thought it was worth having at least one or two non-boolean cases in the tests.

Aha! I wasn't aware. OK, it's grand so

kopf · 2017-11-19T10:16:49Z

tests/test_run.py

+        httsleep.run()
+        assert mock_adapter_send.called
+        args, kwargs = mock_adapter_send.call_args
+    assert args[0].headers == {'conflict-header': 'req-wins', 'req-header': 'myRequest', 'session-header': 'mySession'}


kopf · 2017-11-19T10:18:22Z

tests/test_run.py

+def test_request_verify_overrules_session_verify():
+    """Should give precedence to the 'verify' setting in the request, over that in a specified Session"""
+    session = requests.Session()
+    session.verify = '/path/to/ca/bundle'


Could we not set this to True and then do

httsleep = HttSleeper(URL, {'status_code': 200}, session=session, verify=False)

And then check whether verify is False?

That would of course work, but:

as per my other comment, verify can be a string as well

I thought it improved readability of the test to have the overriding value of /override/path.

I can change it to a boolean case if you'd prefer, just say the word.

Yep, grand, leave it as it is

kopf · 2017-11-19T10:18:56Z

Great stuff!

All that's missing is a changelog entry.

kopf · 2017-11-19T10:36:12Z

And you need to update the documentation - the "Polling" section of tutorial.rst in particular. It would be good to mention the use case (polling using different requests, but inside the same session so that they all get the same headers and cookies). I'd put it straight after the "polling using a POST request" example.

oliverlockwood · 2017-11-19T14:19:23Z

@kopf tutorial & changelog entries now added, please let me know if they need tweaking at all.

coveralls · 2017-11-19T14:19:45Z

Coverage increased (+0.1%) to 95.313% when pulling ff4a601 on oliverlockwood:support-session into 1a5e76d on kopf:master.

kopf · 2017-11-19T14:57:12Z

LGTM, thanks again!

oliverlockwood added 4 commits November 9, 2017 08:51

Add failing tests to cover Session propagation

68a2d10

Add 'session' as an optional kwarg to HttSleeper

bf15b3c

Mock the Adapter send method instead of the Session send, to allow mo…

be665b9

…re meaningful Session testing

Don't send verify=True by default, use None, otherwise it overrides S…

65ec4a4

…ession value

oliverlockwood commented Nov 9, 2017

View reviewed changes

kopf reviewed Nov 19, 2017

View reviewed changes

Add changelog and tutorial entries

ff4a601

oliverlockwood mentioned this pull request Nov 19, 2017

Remove kwargs #15

Merged

kopf merged commit 96ad441 into kopf:master Nov 19, 2017

oliverlockwood deleted the support-session branch November 20, 2017 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support the use of a `Session` object #14

Support the use of a `Session` object #14

oliverlockwood commented Nov 9, 2017

oliverlockwood Nov 9, 2017 •

edited

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

oliverlockwood Nov 9, 2017

kopf Nov 19, 2017

oliverlockwood commented Nov 17, 2017

kopf commented Nov 17, 2017

oliverlockwood commented Nov 17, 2017

kopf Nov 19, 2017

oliverlockwood Nov 19, 2017

kopf Nov 19, 2017

kopf Nov 19, 2017

kopf Nov 19, 2017

oliverlockwood Nov 19, 2017

kopf Nov 19, 2017

kopf commented Nov 19, 2017

kopf commented Nov 19, 2017

oliverlockwood commented Nov 19, 2017

coveralls commented Nov 19, 2017 •

edited

kopf commented Nov 19, 2017

Support the use of a Session object #14

Support the use of a Session object #14

Conversation

oliverlockwood commented Nov 9, 2017

oliverlockwood Nov 9, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverlockwood commented Nov 17, 2017

kopf commented Nov 17, 2017

oliverlockwood commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kopf commented Nov 19, 2017

kopf commented Nov 19, 2017

oliverlockwood commented Nov 19, 2017

coveralls commented Nov 19, 2017 • edited

kopf commented Nov 19, 2017

Support the use of a `Session` object #14

Support the use of a `Session` object #14

oliverlockwood Nov 9, 2017 •

edited

coveralls commented Nov 19, 2017 •

edited