Opened 14 years ago
Closed 13 years ago
#15929 closed Bug (fixed)
test.client.RequestFactory keeps state
Reported by: | 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)
Change History (13)
by , 14 years ago
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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 , 14 years ago
Attachment: | tests-2.py added |
---|
comment:2 by , 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 , 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 , 14 years ago
Attachment: | django-patch-15929.diff added |
---|
comment:4 by , 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 , 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 , 14 years ago
Attachment: | 15929-fix.diff added |
---|
Fix by making AuthenticationMiddleware not do naughty monkey patching
comment:6 by , 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 , 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 , 13 years ago
Needs tests: | unset |
---|
comment:8 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
In [16297]:
(The changeset message doesn't reference this ticket)
Test for the error