Opened 14 years ago

Last modified 11 years ago

#12089 new Bug

test client fails to collect sub-contexts

Reported by: Antti Kaihola Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords: template context TestClient
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If a main template renders a sub-template with a modified or newly created context, test client's response.context fails to include the modified context. An example using {% with %} and {% include %}:

main-template.html

{% with "some-value" as subvariable %}
  {% include sub-template.html %}
{% endwith %}

sub-template.html

The value of subvariable is "{{ subvariable }}".

If the test client requests a view which renders main-template.html, the variable subvariable can't be found in response.context as returned by TestClient.get().

The same happens if a custom template tag creates a new context and renders another template.

Attachments (2)

12089-r11655-patch_test-client-subcontext.diff (4.3 KB ) - added by Antti Kaihola 14 years ago.
patch with tests to fix test client to collect sub-contexts correctly
12089-r11678-patch_test_client_subcontext_v2.diff (6.2 KB ) - added by Antti Kaihola 14 years ago.
improved patch works for complex data (e.g. compiled templates) in the context

Download all attachments as: .zip

Change History (19)

by Antti Kaihola, 14 years ago

patch with tests to fix test client to collect sub-contexts correctly

comment:1 by Antti Kaihola, 14 years ago

Has patch: set

The problem was that the same context object was appended to the ContextList over and over, and if I understand correctly, when the pushed contexts got popped, modified context was forgotten. At least response.context contained a list of multiple references to a single RequestContext object when I examined it with the debugger.

The above patch makes deep copies of the RequestContext objects before adding them to the ContextList of the test client.

Tests are included with the patch. The two template files shown as /dev/null by Trac (apparently it doesn't understand Git's diff format) are /tests/templates/testclient/sub.html and /tests/templates/testclient/super.html, respectively.

comment:2 by Antti Kaihola, 14 years ago

Patch needs improvement: set

There's a problem in the deepcopy approach. I'm trying to understand why it makes a test fail and come up with a better patch.

by Antti Kaihola, 14 years ago

improved patch works for complex data (e.g. compiled templates) in the context

in reply to:  2 comment:3 by Antti Kaihola, 14 years ago

Replying to akaihola:

There's a problem in the deepcopy approach. I'm trying to understand why it makes a test fail and come up with a better patch.

The problem was with Template objects in the context on debug pages. Such context were not deepcopyable. The more manual copying approach in the second version of the patch fixes this problem.

I also included a test case for this issue.

comment:4 by Antti Kaihola, 14 years ago

Patch needs improvement: unset

One thing to note in the v2 patch is that it assumes that all items of
context.dicts have the copy() method. Normally they do as they are
dicts.

In one of my projects I was erroneously calling

render_to_string('template.html', Context({...}))

while the second argument is expected to be a plain dict. This works
because a Context object behaves similarly enough to a dict, but the
patched test client crashed since Context has no copy() method.

One could maybe imagine valid reasons to build a context out of
dict-like objects which aren't real dicts and might lack a
copy() method. Should the test client use more bare-bones dict
methods to make copies of the context to make sure this is as
backwards compatible as possible?

comment:5 by anonymous, 14 years ago

any update on this or roadmap ?

I just found this bug while running a Django TestCase that calls a view which renders a template with a templatetag that has takes_context = True.

in reply to:  5 ; comment:6 by Antti Kaihola, 14 years ago

Keywords: template context TestClient added

Replying to anonymous:

any update on this or roadmap ?

I'm running my own fork while waiting for a review by a core developer, and I haven't brought this up on the IRC channel or the mailing lists.

in reply to:  6 comment:7 by anonymous, 14 years ago

Replying to akaihola:

Replying to anonymous:

any update on this or roadmap ?

I'm running my own fork while waiting for a review by a core developer, and I haven't brought this up on the IRC channel or the mailing lists.

Does your own fork use the patch in this ticket ?

comment:8 by anonymous, 14 years ago

ah, now I notice you're the one who made the patch :)

Any other workarounds? because I want to track the SVN closely.

comment:9 by Antti Kaihola, 14 years ago

Replying to anonymous:

Any other workarounds? because I want to track the SVN closely.

Rebasing with trunk has worked so well that I haven't considered other options. It would be trivial to subclass django.test.Client and override request() to use the patched version of store_rendered_templates(). However, then you might miss changes in trunk to the request() method. In that sense forking and rebasing is a better alternative.

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

Triage Stage: UnreviewedDesign decision needed

I can see what you're trying to do, and the patch makes sense - my concern is the potential for backwards incompatibility. Any template that has a 'with' and 'include' will suddenly acquire a whole lot more contexts in the test output; if you're testing the number of contexts, or if the subcontext introduces a variable that wasn't previously being tested against, you could start to get test failures (or worse, false passes).

in reply to:  10 comment:11 by Antti Kaihola, 14 years ago

Replying to russellm:

I can see what you're trying to do, and the patch makes sense - my concern is the potential for backwards incompatibility. Any template that has a 'with' and 'include' will suddenly acquire a whole lot more contexts in the test output; if you're testing the number of contexts, or if the subcontext introduces a variable that wasn't previously being tested against, you could start to get test failures (or worse, false passes).

Ok. What about these alternatives:

  1. point out the current behavior in the documentation
  2. provide a test client option to activate collecting sub-contexts
  3. add an additional response.deep_context attribute and do complete context collection with sub-contexts there
  4. wait until Django 2.0 and make this change then

How severe is backwards incompatibility in the test framework? It wouldn't break production websites after all.

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

IMHO, Backwards incompatibilities in test suites are even *more* important than backwards incompatibilities in production code. If updating your Django version can cause large portions of your test suite to fail, you still have the problem of working out if the problem is failures in your code, or in your tests.

(3) is probably my preferred option of the 4 you mention, but I'm still not completely sold.

Thinking out loud - it strikes me that a 'sub context' should actually be collected as a 'sub'. That is, if you inspect context[3], and it generated 2 subcontexts, you should be able to inspect context[3].context[0]. I haven't even begun to look into the practicalities of this; I'm just throwing out ideas that make sense to me from a public API point of view.

comment:13 by Julien Phalip, 13 years ago

Severity: Normal
Type: Bug

comment:14 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:15 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:16 by Aymeric Augustin, 11 years ago

Triage Stage: Design decision neededAccepted

Accepting based on Russell's comments. The approach of the current patch is rejected, a backwards-compatible approach such as the one proposed in comment 12 must be found.

comment:17 by Tim Graham, 11 years ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top