Opened 13 years ago
Closed 13 years ago
#16366 closed Bug (fixed)
contrib.auth.tests.context_processors.AuthContextProcessorTests.test_session_not_accessed fails when project loads other middleware that access the session.
Reported by: | Jim Dalton | Owned by: | Aymeric Augustin |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Release blocker | Keywords: | test |
Cc: | Jim Dalton, calvinx, dev@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
AuthContextProcessorTests.test_session_not_accessed()
tests whether the session is accessed when a template is loaded but the auth context processor is not actually accessed in the template. Presumably it's a test that ensures unnecessary session access is avoided.
However, if you have middleware eanbled that access the session at some point in the request cycle, then request.session.accessed will be True going into the test and the test will fail -- even though the code being exercised did not actually hit the session.
As I understand it, the context processors in auth were not actually running until very recently (thanks to r16304), and I"m not sure when this test was actually introduced. I don't have an easy fix for it though. I thought setting request.session.accessed to False prior to calling rendor_to_response
in auth.tests.urls.auth_processor_no_attr_access()
would do the trick -- but that just sets up a false pass. If you set it the same way in auth.tests.urls.auth_processor_attr_access()
then *that* test fails because it checks out to be False.
Does this test have to run in every project? Can it be moved to the main test suite instead of having it be part of the application tests? I'm sure it's valuable to ensure the session is not hit if the context processor is not accessed, but I'm not convinced this behavior will be influenced by any project specific code, so I'm thinking the main test suite is a better place for it. It's definitely a problem to have a test failure in contrib.auth solely because you have middleware that accesses the session.
Attachments (4)
Change History (22)
comment:1 by , 13 years ago
Easy pickings: | set |
---|---|
Has patch: | set |
by , 13 years ago
Attachment: | fix_auth_context_processor_tests.diff added |
---|
comment:2 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Unreviewed → Accepted |
The problem looks valid.
To resolve it, I would probably try to override settings.CONTEXT_PROCESSOR
in the test. Your patch takes a different route. I can't say which approach is the best.
comment:3 by , 13 years ago
Cc: | added |
---|---|
Has patch: | unset |
aaugustin: can you clarify what you mean by "override settings.CONTEXT_PROCESSOR" in this django test?
comment:5 by , 13 years ago
That was a slip, I meant "override settings.MIDDLEWARE_CLASSES
".
See https://docs.djangoproject.com/en/dev/topics/testing/#overriding-settings
comment:7 by , 13 years ago
I just tried override_settings with django 1.4-alpha-1 as below.
I dont know if it comes from override_settings
but this test alone is ok but fails when
./manage.py test aauth.AuthContextProcessorTests
from django.conf import global_settings ... @override_settings(MIDDLEWARE_CLASSES=global_settings.MIDDLEWARE_CLASSES) @override_settings(TEMPLATE_CONTEXT_PROCESSORS=global_settings.TEMPLATE_CONTEXT_PROCESSORS) def test_session_not_accessed(self): """ Tests that the session is not accessed simply by including the auth context processor """ response = self.client.get('/auth_processor_no_attr_access/') self.assertContains(response, "Session not accessed")
comment:8 by , 13 years ago
Has patch: | set |
---|---|
Keywords: | test added |
comment:9 by , 13 years ago
New patch uploaded, as in one of my own projects the test was failing due to a custom context processor.
comment:11 by , 13 years ago
Replying to fbeaufort:
Is this patch going to be into django-1.4 at some point?
If you'd like it to be, you can help. As documented in the contributing docs, the next step for this ticket would be for someone other than the patch author to review the ticket and patch, verify that the code looks correct, the patch applies cleanly, there are no outstanding objections to the approach, and it fixes the reported problem. If so, mark it Ready for Checkin and it will get onto the core devs' radar for commit.
comment:12 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
The previous patch was almost there.
The problem was that django.template.context
caches template context processors that it's seen so overriding the settings didn't have any effect if other tests tests that didn't override the settings were run.
I've updated the patch to clear the cached context processors for those two tests.
I also removed the settings
import because it's not needed.
It would be great if this could be slipped in before the release.
by , 13 years ago
Attachment: | 16366-4.diff added |
---|
follow-up: 17 comment:13 by , 13 years ago
I'd better see the context._standard_context_processors cache emptied in override_settings itself, instead of in each test. Let the committer decide what's the preferred approach.
comment:14 by , 13 years ago
Severity: | Normal → Release blocker |
---|
I'm marking this as a "release blocker" on the basis of Aymeric Augustin's recent message (http://groups.google.com/group/django-developers/browse_thread/thread/cdd0f2b6e2d1141e).
It's not a major issue but I would consider it a regression because Django's tests are failing for some users and a reasonable fix is ready.
Please let me know if this is not the right action.
comment:15 by , 13 years ago
Cc: | added |
---|
comment:16 by , 13 years ago
Owner: | changed from | to
---|
comment:17 by , 13 years ago
Replying to claudep:
I'd better see the context._standard_context_processors cache emptied in override_settings itself, instead of in each test. Let the committer decide what's the preferred approach.
I've created #17787 to track the more general problem. Until a decision is made on that ticket, we can use an ad-hoc reset — we're already doing that in several tests.
Here's a patch that fixes the tests. To test, run project tests with contrib.auth in
INSTALLED_APPS
.If we're going to include these tests in the project tests as now, then the views
auth_processor_no_attr_access()
andauth_processor_attr_access()
inauth.tests.urls
must have a "clean" request object in order for the tests to be meaningful in anyway. By clean I mean no other middleware can have accessed the session (or request.user, which amounts to the same thing) -- otherwise we aren't actually testing what we say we are testing.To resolve this, I did a bit of setup before each of these methods. Basically we reset session.accessed to False, clear the _cache_user from request if it has been set, then rerun the auth middleware on the request object, so that a new lazy user object is added correctly into it. This allows the template calls to do what they claim they are doing and the tests to pass.
I'm marking this as easy-pickings, because it's a pretty straightforward fix and hopefully a straightforward issue.