Opened 7 years ago

Closed 3 years ago

#11077 closed Bug (wontfix)

Django's built-in tests fail when using url reverse tags in default registration templates

Reported by: srosro Owned by: nobody
Component: Testing framework Version:
Severity: Normal Keywords:
Cc: sam@…, r.dav.lc@…, frank@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

To reproduce:
Create a new project. Create some registration templates (ie: templates/password_reset_form.html). Use the {% url %} template tag. For example, {% url index %}. Make sure you have a valid 'index' named url!

You should then get a failure when running the test suite:

#python manage.py test
Creating test database...
Creating table auth_permission
Creating table auth_group
Creating table auth_user
Creating table auth_message
Creating table django_content_type
Creating table django_session
Creating table django_site
Installing index for auth.Permission model
Installing index for auth.Message model
EE.EEEEEEE.F....


...


======================================================================
ERROR: test_confirm_complete (django.contrib.auth.tests.views.PasswordResetTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Applications/Django/django-trunk/django/contrib/auth/tests/views.py", line 71, in test_confirm_complete
    url, path = self._test_confirm_start()
  File "/Applications/Django/django-trunk/django/contrib/auth/tests/views.py", line 31, in _test_confirm_start
    response = self.client.post('/password_reset/', {'email': 'staffmember@example.com'})
  File "/Applications/Django/django-trunk/django/test/client.py", line 299, in post
    return self.request(**r)
  File "/Applications/Django/django-trunk/django/core/handlers/base.py", line 86, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "/Applications/Django/django-trunk/django/contrib/auth/views.py", line 92, in password_reset
    form.save(**opts)
  File "/Applications/Django/django-trunk/django/contrib/auth/forms.py", line 135, in save
    t.render(Context(c)), None, [user.email])
  File "/Applications/Django/django-trunk/django/test/utils.py", line 15, in instrumented_test_render
    return self.nodelist.render(context)
  File "/Applications/Django/django-trunk/django/template/__init__.py", line 768, in render
    bits.append(self.render_node(node, context))
  File "/Applications/Django/django-trunk/django/template/debug.py", line 81, in render_node
    raise wrapped
TemplateSyntaxError: Caught an exception while rendering: Reverse for 'testing1234.index' with arguments '()' and keyword arguments '{}' not found.

Original Traceback (most recent call last):
  File "/Applications/Django/django-trunk/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/Applications/Django/django-trunk/django/template/defaulttags.py", line 387, in render
    args=args, kwargs=kwargs)
  File "/Applications/Django/django-trunk/django/core/urlresolvers.py", line 262, in reverse
    *args, **kwargs)))
  File "/Applications/Django/django-trunk/django/core/urlresolvers.py", line 251, in reverse
    "arguments '%s' not found." % (lookup_view, args, kwargs))
NoReverseMatch: Reverse for 'testing1234.index' with arguments '()' and keyword arguments '{}' not found.

Attachments (3)

test_combine_urlconfs_poc.diff (3.5 KB) - added by Cory Walker 7 years ago.
A proof of concept for the testing framework's URLconf-combining feature
test_combine_urlconfs_docs.patch (6.1 KB) - added by Cory Walker 7 years ago.
Same as before but with documentation
test_combine_urlconfs.diff (8.7 KB) - added by Cory Walker 7 years ago.
The complete patch

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by anonymous

Cc: sam@… added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 7 years ago by Chris Beaven

Component: UncategorizedTesting framework
Resolution: fixed
Status: newclosed
Version: 1.0

I believe this was fixed in response to #10747

comment:3 Changed 7 years ago by Chris Beaven

Either that, or it's a dupe of #10976

comment:4 Changed 7 years ago by Cory Walker

This bug is still present. It's not a duplicate of either of the above mentioned cases (#10747, #10976).

The same exact problem is happening for me using auth and django-registration. It doesn't matter whether I reference the URL using the view or it's name. The problem lies with the fact that the tests for both django.contrib.auth and django-registration set their TestCase.urls setting to override the default URLConf. This problem can be resolved by concatenating the test URL patterns and your root URL patterns. This is an unfeasible solution because it requires modifying the source code. As far as I know, this is the only real solution.

After googling for hours, I've only found two references to this problem besides this ticket:

http://www.mail-archive.com/django-users@googlegroups.com/msg78709.html

http://recurser.com/articles/2009/11/10/noreversematch-custom-registration-templates-break-django-unit-tests/

The only solution they mention is using something like:

{% url home.views.index as index %}{{ index }}

instead of:

{% url home.views.index}

Although this workaround does not require a source code modification to auth or django-registration, it sucks bad because all reversing errors are suppressed whether or not it's during a test. I don't really see the point of testing if things like that need to be done.

Apparently not many people use the test suite because this bug will affect anyone testing with a reverse() or 'url' tag in their template.

Possible solution:
If a TestCase.urls attribute is set, combine it with the ROOT_URLCONF instead of overwriting it. This could be an optional or default feature.

comment:5 Changed 7 years ago by Cory Walker

Resolution: fixed
Status: closedreopened
Triage Stage: UnreviewedDesign decision needed

comment:6 Changed 7 years ago by Cory Walker

I've been thinking a little more about this, and I propose we could use the following options for "./manage.py test"

--combine-urlconfs: tells the testing framework to combine the overridden URLConf with the ROOT_URLCONF. If --combine-urlconfs-with is specified, combine it with that instead of ROOT_URLCONF
--combine-urlconfs-with: the URLConf to use for combining instead of ROOT_URLCONF. This is useful if you need to provide a minimal URL configuration for only the URLs you need to avoid conflicts.

Most of the errors described here are because of auth and django-registration templates including a base.html that reverses to links such as the main index page, a login page, or an about us page (most of them do). --combine-urlconfs-with would be a useful option in that case because it would allow minimalistic URL schemes to be included for just those pages.

Changed 7 years ago by Cory Walker

A proof of concept for the testing framework's URLconf-combining feature

comment:7 Changed 7 years ago by Cory Walker

Has patch: set
Needs documentation: set
Patch needs improvement: set

I added the POC for the feature I described above. I made a few changes to my design, though. Instead of using command line switches, I opted to use variables in settings.py. Using command line switches would have required big changes to the testing framework. These changes are 100% backwards compatible. As long as TEST_COMBINE_URLCONFS is kept False, it will essentially be running the exact same code as before. Please keep in mind that the patch currently has no documentation changes. As far as I know, I need to change these documents:

http://docs.djangoproject.com/en/dev/ref/settings/?from=olddocs#test-name

http://docs.djangoproject.com/en/dev/topics/testing/#django.test.TestCase.urls

I'd also like to do a bit more testing on this. Would anyone mind giving some feedback about this proof of concept? Also, do any tests need to be written for this sort of functionality?

Changed 7 years ago by Cory Walker

Same as before but with documentation

comment:8 Changed 7 years ago by Cory Walker

Needs documentation: unset
Needs tests: set

Changed 7 years ago by Cory Walker

Attachment: test_combine_urlconfs.diff added

The complete patch

comment:9 Changed 7 years ago by Cory Walker

Needs tests: unset
Patch needs improvement: unset
Triage Stage: Design decision neededReady for checkin

I've uploaded a complete version of the patch. I've run many tests on it, using both the Django test suite and my project. I think it's ready to be checked in.

comment:10 Changed 7 years ago by Cory Walker

Triage Stage: Ready for checkinUnreviewed

comment:11 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

There is a broader issue to be decided here - namely, how to set up tests that are isolated from the project settings. The patch on this ticket solves one version of the problem by adding "combine_urls" and "combine_urls_with", but other version are still lurking. Although the symptoms are different, #10976, #10747 and #7611 are all different presentations of the same root problem. We need a consolidated approach that differentiates between integration tests and application tests. Simon started a discussion on django-dev a while back, but it never came to resolution; we need to restart that discussion.

comment:12 Changed 7 years ago by Cory Walker

I agree that the testing framework needs to be more seamless. It works great for Django itself but for actual projects it's sort of clunky. Anyways, the patches I uploaded above are somewhat of a temporary fix if links to other apps are preventing you from testing parts of your project. Since this is for the testing framework, it only needs to be patched on developer copies of Django.

comment:13 Changed 7 years ago by ogai

Small comment on the importance of this bug:

We are implementing a new web service with Django and spent a lot of time trying to figure out why 'manage.py test' were producing so many errors. Luckily we have found this bug report which describes the problem. We think it is important to be fixed, otherwise 'manage.py test' is useless for any project using the url tag in templates.

comment:14 Changed 6 years ago by stringfellow

+1 re: comment from ogai
Very irritating! Making tests pass is currently my sole purpose in life.

comment:15 Changed 6 years ago by berto

+1

I sat down to write some tests on a new application and there's nothing more discouraging than to start with failure. The suggestion by Adam V in the django-dev thread was super quick and easy to avoid the problem ... only test your own apps [1]!

I've peared down the suggestion down to creating a MY_APPS tuple containing only my apps and running the tests with:

./manage.py test $(python -c "import settings; print ' '.join(settings.MY_APPS)")

[1] http://groups.google.com/group/django-developers/browse_thread/thread/c7eb9930591e3a38#msg_36dbc5a388c5fdb2

comment:16 Changed 5 years ago by Julien Phalip

Severity: Normal
Type: Bug

comment:17 Changed 5 years ago by mbitzl

Easy pickings: unset
UI/UX: unset

+1

Any news on this?

comment:18 Changed 5 years ago by Davor Lucic

Cc: r.dav.lc@… added

comment:19 Changed 5 years ago by frank@…

Cc: frank@… added

comment:20 Changed 5 years ago by anonymous

+1, this occurred for us when we tried to over ride the default 500 error template. 500.html which in turn inherited from a base template that contained a % url.

comment:21 Changed 5 years ago by Claude Paroz

I think that some work has been done recently to isolate more closely the settings for contrib apps (e.g. TEMPLATE_DIRS is isolated in contrib.auth). This ticket doesn't seem very useful to me as it doesn't help to track specific issues.

Note that I proposed a different approach that completely isolates the TestCase settings from current project settings in #17194 (see the patch attachment:17194-3.diff:ticket:17194). But finally an other solution was adopted for that ticket.

comment:22 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:23 Changed 4 years ago by Aymeric Augustin

My take on this issue (and on the many similar ones) is that contrib apps tests aren't designed to be run with arbitrary settings, and cannot be made to without:

  • large amounts of work,
  • large override_settings clauses.

Even then, the result will be very fragile, since the CI cannot test all possible settings combinations. It will also be less readable than the current tests. In my opinion, every commit that adds a little piece of test isolation in contrib apps is a loss.

I was willing to do the work of moving the auth tests to tests/ so they won't be run by default, which would solve this problem. But Russell vetoed that. I'll let him propose another solution.

Last edited 4 years ago by Aymeric Augustin (previous) (diff)

comment:24 Changed 4 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

Marking as accepted because it's a bug that needs to be addressed one way or another.

comment:25 Changed 3 years ago by Carl Meyer

I believe this ticket can simply be closed when we merge #17365, since that will change the default behavior of manage.py test so it doesn't run contrib tests, making it feasible for us to simply say "contrib app tests aren't intended to pass with arbitrary settings."

comment:26 Changed 3 years ago by Carl Meyer <carl@…>

In 9012833af857e081b515ce760685b157638efcef:

Fixed #17365, #17366, #18727 -- Switched to discovery test runner.

Thanks to Preston Timmons for the bulk of the work on the patch, especially
updating Django's own test suite to comply with the requirements of the new
runner. Thanks also to Jannis Leidel and Mahdi Yusuf for earlier work on the
patch and the discovery runner.

Refs #11077, #17032, and #18670.

comment:27 Changed 3 years ago by Carl Meyer

Resolution: wontfix
Status: newclosed

Closing wontfix. Since the new test runner does not run contrib app tests by default in user projects, we will no longer be attempting to make contrib app tests pass with arbitrary settings, signals, templates, etc.

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