Code

Opened 5 years ago

Closed 14 months 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 cmwslw 4 years ago.
A proof of concept for the testing framework's URLconf-combining feature
test_combine_urlconfs_docs.patch (6.1 KB) - added by cmwslw 4 years ago.
Same as before but with documentation
test_combine_urlconfs.diff (8.7 KB) - added by cmwslw 4 years ago.
The complete patch

Download all attachments as: .zip

Change History (30)

comment:1 Changed 5 years ago by anonymous

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

comment:2 Changed 5 years ago by SmileyChris

  • Component changed from Uncategorized to Testing framework
  • Resolution set to fixed
  • Status changed from new to closed
  • Version 1.0 deleted

I believe this was fixed in response to #10747

comment:3 Changed 5 years ago by SmileyChris

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

comment:4 Changed 4 years ago by cmwslw

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 4 years ago by cmwslw

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Design decision needed

comment:6 Changed 4 years ago by cmwslw

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 4 years ago by cmwslw

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

comment:7 Changed 4 years ago by cmwslw

  • 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 4 years ago by cmwslw

Same as before but with documentation

comment:8 Changed 4 years ago by cmwslw

  • Needs documentation unset
  • Needs tests set

Changed 4 years ago by cmwslw

The complete patch

comment:9 Changed 4 years ago by cmwslw

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Design decision needed to Ready 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 4 years ago by cmwslw

  • Triage Stage changed from Ready for checkin to Unreviewed

comment:11 Changed 4 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design 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 4 years ago by cmwslw

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

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

comment:15 Changed 4 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 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:17 Changed 3 years ago by mbitzl

  • Easy pickings unset
  • UI/UX unset

+1

Any news on this?

comment:18 Changed 3 years ago by rebus

  • Cc r.dav.lc@… added

comment:19 Changed 3 years ago by frank@…

  • Cc frank@… added

comment:20 Changed 3 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 3 years ago by claudep

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 16 months ago by aaugustin

  • Status changed from reopened to new

comment:23 Changed 16 months ago by aaugustin

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 16 months ago by aaugustin (previous) (diff)

comment:24 Changed 16 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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

comment:25 Changed 14 months ago by carljm

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 14 months 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 14 months ago by carljm

  • Resolution set to wontfix
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.