Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#25848 closed Cleanup/optimization (fixed)

Django 1.9 breaks django_coverage_plugin, template coverage doesn't work

Reported by: Ned Batchelder Owned by: nobody
Component: Template system Version: 1.9
Severity: Normal Keywords:
Cc: sdeibel@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

django_coverage_plugin (https://github.com/nedbat/django_coverage_plugin) does some tricky things to figure out which lines in your templates are being used, and which are not. It works great in Django 1.4 through 1.8. The template refactoring in 1.9 removed information the plugin needs to work.

Preston Timmons has been very helpful in diagnosing the problem. He says the commit that broke it is: https://github.com/django/django/commit/55f12f8709f0604df7e1817a4c114ead1fb9a311

Preston's fuller assessment is:

Yes, I'm supportive of that going into 1.9.1. Do you want to make a ticket in
Trac? Please point to the commit where I broke things for you and mention how
token.source used to give you access to the current template, whereas
context.template.origin only gives you access to the parent. This was a change
in private API, but you have an important use case that's worth making sure you
can access the information you need.
.
A proper fix may include changing the semantics of context.bind_template.
Alternatively, we could expose the current template on context.render_context.
That attribute exists to store state during render invocations.
.
A less pleasant possibility may be to access compiled_parent.origin.name in
the ExtendsNode render method. Or to grab the latest origin from the extends
history with context.render_context[ExtendsNode.context_key][-1]. The
IncludeNode would require a similar hack, but hopefully we can give you
something better to work with than that.

I'm very willing to work out a good solution, and do some of the work on the Django side if need be. It would be a shame if 1.9 had to sit out this feature of coverage.py.

Change History (11)

comment:1 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

I'll be happy to backport the solution to 1.9.

comment:2 by Tim Graham, 8 years ago

As an aside, I wonder if you tried to test with the prereleases of Django 1.9 and if not, if there's any way we could have helped to better facilitate that? If it's just a lack of time on your end, I certainly understand that.

comment:3 by Preston Timmons, 8 years ago

I'm not confident if these are the right solutions yet, but I have a couple ideas:

First, we could update Template.render to look like:

class Template(object):

    def _render(self, context):
        with context.bind_render_template(self):
            self.nodelist.render(context)

This would set a new attribute on context, render_template. This would reference the template of the current render call. This change would need to occur in _render rather than render to work with both ExtendsNode and IncludeNode.

Alternatively, we could update ExtendsNode and BlockContext to store the origin along with each node. When Ned encounters a BlockNode, he could find the template origin by looking it up in the block context.

comment:4 by Ned Batchelder, 8 years ago

Tim, thanks. In this case, it was mostly just poor timing between when my attention came back around to Django template coverage, and when 1.9 came out. But partly, it was not realizing that template changes were underway, and being complacent that since my code was working on 1.4 through 1.8, it would probably be fine. :)

comment:5 by Ned Batchelder, 8 years ago

Preston, I'm assuming that you will make a proposal about the best way to proceed. I don't know enough to have an opinion between the two ideas you've laid out here. Or, if you need me to do more, let me know.

comment:6 by Ned Batchelder, 8 years ago

We missed the possibility of getting into 1.9.1. Preston, what should I expect from you? I'm not trying to nag, I know what it's like to be on the other side of that question! :)

Is 1.9.2 do-able?

comment:7 by Stephan Deibel, 8 years ago

FWIW I've run into this problem also in Wing IDE's support for debugging Django templates. I was able to partially solve the problem by looking up the stack for the nearest frame in Template and using self.origin.name. However, when a block is overridden via 'extends' I've not been able to figure out how to get the correct file name.

For example context.render_context[ExtendsNode.context_key][-1].name is still the wrong template.

Is there any reason that the template name can't just be put into self.token? Currently you have token.position but not token.filename and it seems position is pretty useless w/o the filename, as far as I can tell.

Last edited 8 years ago by Stephan Deibel (previous) (diff)

comment:8 by Stephan Deibel, 8 years ago

Cc: sdeibel@… added

comment:9 by Tim Graham, 8 years ago

Has patch: set

comment:10 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In cfda1fa3:

Fixed #25848 -- Set template origin on each node.

Prior to 55f12f8709, the template origin was available on each node via
self.token.source[0]. This behavior was removed when debug handling was
simplified, but 3rd-party debugging tools still depend on its presence.
This updates the Parser to set origin on individual nodes. This enables the
source template to be determined even when template extending or including is
used.

comment:11 by Tim Graham <timograham@…>, 8 years ago

In 218cc71:

[1.9.x] Fixed #25848 -- Set template origin on each node.

Prior to 55f12f8709, the template origin was available on each node via
self.token.source[0]. This behavior was removed when debug handling was
simplified, but 3rd-party debugging tools still depend on its presence.
This updates the Parser to set origin on individual nodes. This enables the
source template to be determined even when template extending or including is
used.

Backport of cfda1fa3f8d95f0f4a369da9021dbd770e5fa44a from master

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