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

treq.testing.StubTreq: fix persisting twisted.web.server.Session objects between requests #328

Merged
merged 5 commits into from May 13, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions changelog.d/327.bugfix.rst
@@ -0,0 +1 @@
``treq.testing.StubTreq`` now persists ``twisted.web.server.Session`` instances between requests.
8 changes: 8 additions & 0 deletions docs/api.rst
Expand Up @@ -114,6 +114,14 @@ StubTreq Objects
asynchronous. In that case, after each write from the server,
:meth:`flush()` must be called so the client can see it.

.. method:: cleanSessions()

Clean up sessions to prevent leaving behind a dirty reactor.

If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session`
objects, you most likely have to call this method once you are done,
for example during the tearDown of a unittest TestCase.

As the methods on :class:`treq.client.HTTPClient`:

.. method:: request
Expand Down
3 changes: 3 additions & 0 deletions docs/testing.rst
Expand Up @@ -57,3 +57,6 @@ This is superior to calling your resource's methods directly or passing mock obj
Thus, the ``request`` object your code interacts with is a *real* :class:`twisted.web.server.Request` and behaves the same as it would in production.

Note that if your resource returns :data:`~twisted.web.server.NOT_DONE_YET` you must keep a reference to the :class:`~treq.testing.RequestTraversalAgent` and call its :meth:`~treq.testing.RequestTraversalAgent.flush()` method to spin the memory reactor once the server writes additional data before the client will receive it.

If you are using :class:`~twisted.web.server.Session` instances in the wrapped resource, you may need to call :meth:`~treq.testing.StubTreq.cleanSessions()` to expire all sessions and prevent an unclean/dirty reactor.

45 changes: 45 additions & 0 deletions src/treq/test/test_testing.py
Expand Up @@ -52,6 +52,18 @@ def render(self, request):
return NOT_DONE_YET


class _SessionIdTestResource(Resource):
"""
Resource that returns the current session ID.
"""
isLeaf = True

def render(self, request):
session = request.getSession()
uid = session.uid
return uid


class StubbingTests(TestCase):
"""
Tests for :class:`StubTreq`.
Expand Down Expand Up @@ -242,6 +254,39 @@ def test_handles_successful_asynchronous_requests_with_streaming(self):
stub.flush()
self.successResultOf(d)

def test_session_persistence_between_requests(self):
"""
Calling request.getSession() in the wrapped resource will return
a session with the same ID, until the sessions are cleaned.
"""
rsrc = _SessionIdTestResource()
stub = StubTreq(rsrc)
# request 1, getting original session ID
d = stub.request("method", "http://example.com/")
resp = self.successResultOf(d)
cookies = resp.cookies()
sid_1 = self.successResultOf(resp.content())
# request 2, ensuring session ID stays the same
d = stub.request("method", "http://example.com/", cookies=cookies)
resp = self.successResultOf(d)
sid_2 = self.successResultOf(resp.content())
self.assertEqual(sid_1, sid_2)
# request 3, ensuring the session IDs are different after cleaning
# or expiring the sessions
stub.cleanSessions()
d = stub.request("method", "http://example.com/")
resp = self.successResultOf(d)
cookies = resp.cookies()
sid_3 = self.successResultOf(resp.content())
self.assertNotEqual(sid_1, sid_3)
# request 4, ensuring that once again the session IDs are the same
d = stub.request("method", "http://example.com/", cookies=cookies)
resp = self.successResultOf(d)
sid_4 = self.successResultOf(resp.content())
self.assertEqual(sid_3, sid_4)
# when done, clean sessions to avoid leaving a dirty reactor behind
stub.cleanSessions()


class HasHeadersTests(TestCase):
"""
Expand Down
20 changes: 15 additions & 5 deletions src/treq/testing.py
Expand Up @@ -88,6 +88,7 @@ def __init__(self, rootResource):
reactor=self._memoryReactor,
endpointFactory=_EndpointFactory(self._memoryReactor))
self._rootResource = rootResource
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor)
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that you can cause the session to use the correct reactor by overriding sessionFactory here like this:

Suggested change
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor)
self._serverFactory = Site(self._rootResource, reactor=self._memoryReactor)
self._serverFactory.sessionFactory = lambda site, uid: Session(site, uid, reactor=self._memoryReactor)

Then you don't need the cleanup code in the test you added, and more importantly you don't need cleanup code in any test that uses RequestTraversalAgent.

self._pumps = set()

def request(self, method, uri, headers=None, bodyProducer=None):
Expand Down Expand Up @@ -126,8 +127,7 @@ def check_already_called(r):
# Create the protocol and fake transport for the client and server,
# using the factory that was passed to the MemoryReactor for the
# client, and a Site around our rootResource for the server.
serverFactory = Site(self._rootResource, reactor=self._memoryReactor)
serverProtocol = serverFactory.buildProtocol(clientAddress)
serverProtocol = self._serverFactory.buildProtocol(clientAddress)
serverTransport = iosim.FakeTransport(
serverProtocol, isServer=True,
hostAddress=serverAddress, peerAddress=clientAddress)
Expand Down Expand Up @@ -228,8 +228,8 @@ def __init__(self, resource):
:param resource: A :obj:`Resource` object that provides the fake
responses
"""
_agent = RequestTraversalAgent(resource)
_client = HTTPClient(agent=_agent,
self._agent = RequestTraversalAgent(resource)
_client = HTTPClient(agent=self._agent,
data_to_body_producer=_SynchronousProducer)
for function_name in treq.__all__:
function = getattr(_client, function_name, None)
Expand All @@ -239,7 +239,17 @@ def __init__(self, resource):
function = _reject_files(function)

setattr(self, function_name, function)
self.flush = _agent.flush
self.flush = self._agent.flush

def cleanSessions(self):
"""
Clean up sessions to prevent leaving behind a dirty reactor.
Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior of leaving a dirty reactor looks like a bug in Twisted to me, actually. The session's delayed call should be registered with the reactor passed to Site.

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed https://twistedmatrix.com/trac/ticket/10177 for this issue.

I think that it could be worked around here by overriding the site's sessionFactory to pass the memory reactor to the Session constructor. Then you wouldn't need this method.

If you are using :obj:`StubTreq` with :obj:`twisted.web.server.Session`
objects, you most likely have to call this method once you are done,
for example during the tearDown of a unittest TestCase.
Copy link
Contributor

Choose a reason for hiding this comment

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

For stuff like this using self.addCleanup is almost always superior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twm Are you suggesting replacing this whole method to somehow use said function or to change the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

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

To change the documentation, but it's a moot point if this method is going away.

"""
for sid in list(self._agent._serverFactory.sessions.keys()):
self._agent._serverFactory.sessions[sid].expire()


class StringStubbingResource(Resource):
Expand Down