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

[bennr01]

This PR contains my proposed fix for #327. For details about the issue and this fix, see said issue.

tl;dr of original issue: Previously, a resource wrapped by treq.testing.StubTreq could not properly use request.getSession(), as it would always create a new twisted.web.server.Session instead of returning the same one. This is because treq.testing.StubTreq always create a new instance of twisted.web.server.Site for each request. As sessions are stored within a Site instance, this meant that upon completion of a request, the original session was lost. This PR solves this issue by moving the twisted.web.server.Site instance to an attribute of the agent. Additionally, the new treq.testing.StubTreq.cleanSessions() method provides a simple way of expiring the sessions in order to avoid leaving behind a dirty/unclean reactor. Although I am not sure if this is the best way to do this.

[twm]

Thank you for the well-stated bug report and nicely polished PR! I think that the overall approach of reusing the Site is reasonable.

Would you find the cleanSessions method useful if it weren't necessary?

[bennr01]

Hi,

Would you find the cleanSessions method useful if it weren't necessary?

It depends, but I'm leaning towards "no". It is useful when using treq.testing.StubTreq for testing twisted.web applications and wanting to reset sessions between requests, but there may be better ways to do that. During debugging I encountered some weird cookie behavior which I am not sure is intentional (the same cookies being used between requests, even when no cookies parameter was passed to the request or the .clear() was called on the cookies), so maybe it's not necessary even when one wants to have a new session during the same test_* function.

Still, the primarily concern for writing .cleanSessions() was to provide a way to avoid having the user access private (as in 'starting with a underscore') attributes and methods. So if there would be a better way I'd be in favor of dropping .cleanSessions(). It's an explicit call for something that should be (IMO) implicitly cleaned up.

Personally, I also feel like .cleanSessions() is too specific for what it's supposed to do. A more generic method for completely resetting all associated states would be a more clean interface.

[bennr01]

Hi,

I have now removed .cleanSessions() again. Instead, I've moved the code directly into the test with a note to remove it once ticket 10177 is fixed.

[bennr01]

I've implemented the changes proposed above:

• treq.testing.RequestTraversalAgent will now modify self._server.sessionFactory to always use the memory reactor (though seeing https://github.com/twisted/twisted/pull/1587 this may have been uneccessary).
• treq.test.test_testing._SessionIdTestResource will now keep track of the sessions it created and provide a method for expiring these sessions.

[bennr01]

@twm Oops, I forgot that I configured the flake8 I'm using to ignore E501... Should be fixed now.