Opened 14 years ago

Closed 13 years ago

#15929 closed Bug (fixed)

test.client.RequestFactory keeps state

Reported by: m.vantellingen@… Owned by: nobody
Component: Testing framework Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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).

Attachments (5)

tests.py (735 bytes ) - added by m.vantellingen@… 14 years ago.
Test for the error
tests-2.py (1.0 KB ) - added by Aymeric Augustin 14 years ago.
django-patch-15929.diff (760 bytes ) - added by Michael 14 years ago.
15929-fix.diff (840 bytes ) - added by Luke Plant 14 years ago.
Fix by making AuthenticationMiddleware not do naughty monkey patching
15929.diff (2.3 KB ) - added by Ramiro Morales 13 years ago.
Fix bu Luke Plant integrated with tests by m.vantellingen AT auto-interactive.nl and aaugustin

Download all attachments as: .zip

Change History (13)

by m.vantellingen@…, 14 years ago

Attachment: tests.py added

Test for the error

comment:1 by Aymeric Augustin, 14 years ago

Triage Stage: UnreviewedAccepted

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.

by Aymeric Augustin, 14 years ago

Attachment: tests-2.py added

comment:2 by Aymeric Augustin, 14 years ago

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.

comment:3 by Michael, 14 years ago

Has patch: set

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

by Michael, 14 years ago

Attachment: django-patch-15929.diff added

comment:4 by Aymeric Augustin, 14 years ago

Needs tests: set

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?

comment:5 by Michael, 14 years ago

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.


by Luke Plant, 14 years ago

Attachment: 15929-fix.diff added

Fix by making AuthenticationMiddleware not do naughty monkey patching

comment:6 by Luke Plant, 14 years ago

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.

by Ramiro Morales, 13 years ago

Attachment: 15929.diff added

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

comment:7 by Ramiro Morales, 13 years ago

Needs tests: unset

comment:8 by Luke Plant, 13 years ago

Resolution: fixed
Status: newclosed

In [16297]:

(The changeset message doesn't reference this ticket)

Note: See TracTickets for help on using tickets.
Back to Top