Opened 15 years ago

Closed 11 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 14 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 14 years ago.
Same as before but with documentation
test_combine_urlconfs.diff (8.7 KB ) - added by Cory Walker 14 years ago.
The complete patch

Download all attachments as: .zip

Change History (30)

comment:1 by anonymous, 15 years ago

Cc: sam@… added

comment:2 by Chris Beaven, 15 years ago

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

I believe this was fixed in response to #10747

comment:3 by Chris Beaven, 15 years ago

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

comment:4 by Cory Walker, 14 years ago

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 by Cory Walker, 14 years ago

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

comment:6 by Cory Walker, 14 years ago

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.

by Cory Walker, 14 years ago

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

comment:7 by Cory Walker, 14 years ago

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?

by Cory Walker, 14 years ago

Same as before but with documentation

comment:8 by Cory Walker, 14 years ago

Needs documentation: unset
Needs tests: set

by Cory Walker, 14 years ago

Attachment: test_combine_urlconfs.diff added

The complete patch

comment:9 by Cory Walker, 14 years ago

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 by Cory Walker, 14 years ago

Triage Stage: Ready for checkinUnreviewed

comment:11 by Russell Keith-Magee, 14 years ago

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 by Cory Walker, 14 years ago

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 by ogai, 14 years ago

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 by stringfellow, 14 years ago

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

comment:15 by berto, 13 years ago

+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 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:17 by mbitzl, 13 years ago

Easy pickings: unset
UI/UX: unset

+1

Any news on this?

comment:18 by Davor Lucic, 13 years ago

Cc: r.dav.lc@… added

comment:19 by frank@…, 13 years ago

Cc: frank@… added

comment:20 by anonymous, 12 years ago

+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 by Claude Paroz, 12 years ago

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 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:23 by Aymeric Augustin, 11 years ago

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 11 years ago by Aymeric Augustin (previous) (diff)

comment:24 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

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

comment:25 by Carl Meyer, 11 years ago

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 by Carl Meyer <carl@…>, 11 years ago

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 by Carl Meyer, 11 years ago

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