#16412 closed Cleanup/optimization (fixed)
Disabling an 'django.contrib.sites' app, causes error in tests with django.contrib.auth.tests.forms.PasswordResetFormTest
Reported by: | Matt Harasymczuk | Owned by: | nobody |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | djangoproject.com@… | 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 (last modified by )
Try disabling an 'django.contrib.sites' app, (which as it is a contrib app, should be an optional), and you'll get an error in tests with django.contrib.auth.tests.forms.PasswordResetFormTest.
Attachments (1)
Change History (15)
comment:1 by , 13 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 13 years ago
I am not sure, I have been using contrib.auth without contrib.sites for a few years now.
comment:3 by , 13 years ago
Hmm, maybe I'm wrong then. Feel free to re-open but you should provide more information including the traceback from the failing test.
comment:4 by , 13 years ago
Resolution: | invalid |
---|---|
Status: | closed → reopened |
Creating test database for alias 'default'... ..............................................................E................. ................................................................................ ................................................................................ ................................................................................ ...... ====================================================================== ERROR: test_custom_email_subject (django.contrib.auth.tests.forms.PasswordResetFormTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "C:\Python27\Lib\site-packages\django\contrib\auth\tests\forms.py", line 264, in test_custom_email_subject form.save() File "C:\Python27\Lib\site-packages\django\contrib\auth\forms.py", line 138, in save current_site = get_current_site(request) File "C:\Python27\Lib\site-packages\django\contrib\sites\models.py", line 94, in get_current_site current_site = RequestSite(request) File "C:\Python27\Lib\site-packages\django\contrib\sites\models.py", line 74, in __init__ self.domain = self.name = request.get_host() AttributeError: 'NoneType' object has no attribute 'get_host' ---------------------------------------------------------------------- Ran 326 tests in 5.113s FAILED (errors=1) Destroying test database for alias 'default'...
comment:5 by , 13 years ago
it is fully functional without django.contrib.sites app, no errors, but this one test fails.
comment:6 by , 13 years ago
What depends on the contrib.sites contrib apps is the contrib.auth.forms module. It changed in [13980] but it it already depended on it.
What tests are you running?
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
Has patch: | set |
Severity: | Release blocker → Normal |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
django.contrib.auth
uses the get_current_site
method of django.contrib.sites
, which is safe to use, whether the sites framework is or isn't enabled.
However, when the sites framework isn't enabled, this method must receive the request
in argument. In test_custom_email_subject
, there is no request available, and the test crashes. Attached patch fixes this (and avoids relying on the default name of the default site).
by , 13 years ago
Attachment: | 16412.diff added |
---|
comment:9 by , 13 years ago
Needs tests: | set |
---|
comment:10 by , 13 years ago
Why does this need tests when the diff fixes a failing test (TDD: "...first the developer writes a failing automated test case..."), and the change is 1 line.
comment:11 by , 13 years ago
Description: | modified (diff) |
---|
comment:12 by , 13 years ago
Needs tests: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
True, I had been a bit too quick in my initial judgment and there is no need to write a test here. aaugustin made a good analysis of the issue and the patch seems reasonable. Also the domain_override
attribute doesn't seem to be tested yet so that will increase the test suite's coverage. A comment should just be added to that line of code explaining that domain_override
needs to be provided in the absence of a request object in case contrib.sites
isn't installed -- this can be done on commit.
comment:14 by , 13 years ago
Description: | modified (diff) |
---|---|
Summary: | Disabling an 'django.contrib.sites' app, causes Try disabling an 'django.contrib.sites' app, (which as it is a contrib app, should be an optional), and you'll get an error in tests with django.contrib.auth.tests.forms.PasswordResetFormTest → Disabling an 'django.contrib.sites' app, causes error in tests with django.contrib.auth.tests.forms.PasswordResetFormTest |
(just fixing the mess I made with the description and summary of this ticket)
While the contrib apps should be optional, they do depend on each other. contrib.auth requires contrib.sites.