Code

Opened 4 years ago

Last modified 8 months ago

#12089 new Bug

test client fails to collect sub-contexts

Reported by: akaihola Owned by: nobody
Component: Testing framework Version: master
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 akaihola 4 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 akaihola 4 years ago.
improved patch works for complex data (e.g. compiled templates) in the context

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by akaihola

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

comment:1 Changed 4 years ago by akaihola

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 follow-up: Changed 4 years ago by akaihola

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

Changed 4 years ago by akaihola

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

comment:3 in reply to: ↑ 2 Changed 4 years ago by akaihola

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

  • 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 follow-up: Changed 4 years ago by 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.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by akaihola

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

comment:7 in reply to: ↑ 6 Changed 4 years ago by 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 ?

comment:8 Changed 4 years ago by anonymous

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

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 follow-up: Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Design 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).

comment:11 in reply to: ↑ 10 Changed 4 years ago by akaihola

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

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 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:14 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:15 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:16 Changed 13 months ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

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 Changed 8 months ago by timo

  • Patch needs improvement set

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.