test.client.RequestFactory keeps state

[m.vantellingen@…]

See the attached testcase. When a unittest calls self.client.get() once all futher calls to RequestFactory.get() result in different request objects (the user and session are available).

[m.vantellingen@…]

Test for the error

[Aymeric Augustin]

I confirm the issue. I attach a slightly modified TestCase that works out of the box and makes the problem more obvious.

Removing either self.client.get('/') or request.session = {} from common_test_that_should_always_pass make the problem disappear. It looks like there is some shared state between the TestClient and the RequestFactory, where the docs only mention a shared API.

The state leakage between the tests is probably related, and it's pretty bad, since the same test can pass or fail depending on its position in the TestCase.

[Aymeric Augustin]

Oops, §2 above should read:

Removing either self.client.get('/') from test_request_after_client or request.session = {} from common_test_that_should_always_pass makes the problem disappear.

[Michael]

The issue seems to be in the AuthenticationMiddleware which patches the WSGIRequest class to add the LazyUser() object. The simplest solution seems to be to remove the user attribute on the created request in the RequestFactory. See attached patch

[Aymeric Augustin]

I haven't looked into the details, but this patch looks like it's fixing the symptoms and not addressing the root cause of the problem.

For instance, what if someone has a custom middleware that adds another variable?

[Michael]

Well the root cause is obviously that the AuthenticationMiddleware modifies the WSGIRequest class.
The proper solution is to actually not do this, but there are probably reasons why this is done :-)

I'm not sure if django should support custom middleware which modifies django internals.

[Luke Plant]

Fix by making AuthenticationMiddleware not do naughty monkey patching

[Luke Plant]

The authentication does indeed patch the request class for a reason, but there is way to avoid it, which is in the patch I just attached. I haven't actually checked that my patch fixes the issue, but I'm pretty sure it does, and all the other tests pass.

I'm happy with this approach, but not with just fixing the symptoms as in the alternative approach. If someone integrates the regression tests for this into a full patch, I'll mark it as RFC.

[Ramiro Morales]

Fix bu Luke Plant integrated with tests by m.vantellingen AT auto-interactive.nl and aaugustin

[Luke Plant]

In [16297]:

(The changeset message doesn't reference this ticket)