test client fails to collect sub-contexts

[Antti Kaihola]

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.

[Antti Kaihola]

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

[Antti Kaihola]

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.

[Antti Kaihola]

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.

[Antti Kaihola]

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

[Antti Kaihola]

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.

[Antti Kaihola]

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?

[anonymous]

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.

[Antti Kaihola]

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.

[anonymous]

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 ?

[anonymous]

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

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

[Antti Kaihola]

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.

[Russell Keith-Magee]

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).

[Antti Kaihola]

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.

[Russell Keith-Magee]

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.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Aymeric Augustin]

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.