Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#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: master
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 Ramiro Morales)

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)

16412.diff (613 bytes) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by Dougal Matthews

Resolution: invalid
Status: newclosed

While the contrib apps should be optional, they do depend on each other. contrib.auth requires contrib.sites.

comment:2 Changed 5 years ago by Matt Harasymczuk

I am not sure, I have been using contrib.auth without contrib.sites for a few years now.

comment:3 Changed 5 years ago by Dougal Matthews

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 Changed 5 years ago by Matt Harasymczuk

Resolution: invalid
Status: closedreopened

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

Version 0, edited 5 years ago by Matt Harasymczuk (next)

comment:5 Changed 5 years ago by Matt Harasymczuk

it is fully functional without django.contrib.sites app, no errors, but this one test fails.

comment:6 Changed 5 years ago by Ramiro Morales

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:7 Changed 5 years ago by Matt Harasymczuk

python manage.py test in my project root directory

comment:8 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset
Has patch: set
Severity: Release blockerNormal
Triage Stage: UnreviewedAccepted
Type: BugCleanup/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).

Changed 5 years ago by Aymeric Augustin

Attachment: 16412.diff added

comment:9 Changed 5 years ago by Julien Phalip

Needs tests: set

comment:10 Changed 5 years ago by anonymous coward

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 Changed 5 years ago by Ramiro Morales

Description: modified (diff)

comment:12 Changed 5 years ago by Julien Phalip

Needs tests: unset
Triage Stage: AcceptedReady 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:13 Changed 5 years ago by Julien Phalip

Resolution: fixed
Status: reopenedclosed

In [16717]:

Fixed #16412 -- Prevented a contrib.auth test from failing in the potential case where contrib.sites was not installed. Thanks to haras for the report and to Aymeric Augustin for the patch.

comment:14 Changed 5 years ago by Ramiro Morales

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

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