Opened 10 years ago
Closed 10 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)
Changed 10 years ago by
comment:1 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | tests-2.py added |
---|
comment:2 Changed 10 years ago by
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 Changed 10 years ago by
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
Changed 10 years ago by
Attachment: | django-patch-15929.diff added |
---|
comment:4 Changed 10 years ago by
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 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | 15929-fix.diff added |
---|
Fix by making AuthenticationMiddleware not do naughty monkey patching
comment:6 Changed 10 years ago by
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.
Changed 10 years ago by
Attachment: | 15929.diff added |
---|
Fix bu Luke Plant integrated with tests by m.vantellingen AT auto-interactive.nl and aaugustin
comment:7 Changed 10 years ago by
Needs tests: | unset |
---|
comment:8 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
In [16297]:
(The changeset message doesn't reference this ticket)
Test for the error