Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#17529 closed Bug (fixed)

get_template_from_string default arguments break assertTemplateUsed

Reported by: Ian Clelland Owned by: Unai Zalakain
Component: Template system Version: dev
Severity: Normal Keywords: assertTemplateUsed get_template_from_string template testing
Cc: claude@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

django.template.loader.get_template_from_string only requires a single argument -- the template source string. The origin and name parameters are optional, and default to None. (This function is never called in the Django codebase with missing parameters, except in the test suite)

If the name parameter is left out, it is explicitly passed as None to the Template constructor, and so the new template receives a name of None, rather than the default "<Unknown Template>".

django.test.testcases.TransactionTestCase.assertTemplateUsed contains an assertion which joins the 'name' parameter of all of the templates in the request. If any templates used were constructed without a name, the None in the list will cause a TypeError before the assertion can even be tested.

This is the simplest project I can create which exemplifies this problem:

templatetest/urls.py:

from django.conf.urls import patterns, include, url

urlpatterns = patterns('',
    url(r'^test/$', 'templatetest.views.test_view', name='test'),
)

templatetest/views.py:

from django.template import loader, Context
from django.http import HttpResponse

def test_view(request):
    template = loader.get_template_from_string("This is a string-based template")
    return HttpResponse(template.render(Context({})))

templatetest/tests.py:

from django.test import TestCase

class TemplateFromStringTest(TestCase):
    def test_none_template_used(self):
        response = self.client.get('/test/')
        self.assertTemplateUsed(response, "base.html")

./manage.py test templatetest:

Creating test database for alias 'default'...
E
======================================================================
ERROR: test_template_used (templatetest.tests.TemplateFromStringTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ian/Code/Tests/django-template-test/templatetest/templatetest/tests.py", line 6, in test_template_used
    self.assertTemplateUsed(response, "base.html")
  File "/Users/ian/.virtualenvs/django-trunk/lib/python2.7/site-packages/django/test/testcases.py", line 629, in assertTemplateUsed
    (template_name, u', '.join(template_names)))
TypeError: sequence item 0: expected string or Unicode, NoneType found

----------------------------------------------------------------------
Ran 1 test in 0.133s

FAILED (errors=1)
Destroying test database for alias 'default'...

I believe that either assertTemplateUsed needs to guard against None values appearing in the list of template names, or get_template_from_string needs to be modified to callTemplate.__init__ in a way which uses *that* class's default parameters, or possibly both. I'll work up a quick patch once I get a feel for the right place to fix this.

Change History (7)

comment:1 by Ian Clelland, 13 years ago

Component: Testing frameworkTemplate system
Version: 1.3SVN

comment:2 by Claude Paroz, 13 years ago

Cc: claude@… added
Triage Stage: UnreviewedAccepted

I would be tempted to fix it this way in assertTemplateUsed:

  -      template_names = [t.name for t in response.templates]
  +      template_names = [t.name for t in response.templates if t.name]
         if not template_names:
  -         self.fail(msg_prefix + "No templates used to render the response")
  +         self.fail(msg_prefix + "No named templates used to render the response")

comment:3 by Ian Clelland, 13 years ago

Claude, that's in fact how I've handled it locally, in a TransactionTestCase subclass -- my only reservation is that this still leaves a difference between the objects returned from

t = loader.get_template_from_string("template source")

and

t = Template("template source")

So I'm torn between considering this a bug in the testing framework and a bug in the template system.

comment:4 by Claude Paroz, 13 years ago

I see your point. It's about the semantic of the name property of a template created from a direct string parameter, None or '<Unknown Template>'. I would vote for replacing '<Unknown Template>' by None in the Template __init__ which seems a slightly more pythonic way of stating that a property is unset. I've run the entire test suite with this change without failure.

FWIW, I traced the adding of this code: changeset:3707.

comment:5 by Unai Zalakain, 11 years ago

Has patch: set
Owner: changed from nobody to Unai Zalakain
Status: newassigned

PR sent: https://github.com/django/django/pull/1867

Do I have to note this change in the release notes? It's kind of insignificant.

comment:6 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 72f63bd24d44b5a4f24ad8fa27ebba96d9a507d8:

Fixed #17529 -- get_template_from_string default arguments break

get_template_from_string default arguments were breaking
assertTemplateUsed. The solution has been to return only the names of the
templates with a name attribute distinct of None. The default name
kwarg of Template has been changed to None, more pythonic than '<Unknown
Template>'.

comment:7 by Unai Zalakain, 11 years ago

Great, thanks!

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