Code

Opened 4 years ago

Last modified 4 years ago

#14365 new New feature

Make template-rendering signals available also in DEBUG mode

Reported by: carljm Owned by: carljm
Component: Testing framework Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Django's test setup monkeypatches the template renderer to send a signal every time a template is rendered. The test client handles this signal to provide template-rendered debugging information.

It would be useful to have access to this signal in DEBUG mode, not only in testing, so that the test Client can be used from the python shell without losing the template info, and so external tools like django-debug-toolbar can make use of it without having to monkeypatch.

The original reason for the current behavior was performance; signals used to be quite slow. This is no longer the case (and it should be possible to make the performance impact in production nothing more than a single DEBUG check at startup/init time). Nonetheless, any patch here should come with performance impact data.

Attachments (2)

14365_r13962.diff (6.1 KB) - added by carljm 4 years ago.
14365_djangobench.txt (2.1 KB) - added by carljm 4 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 4 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from 1.2 to SVN

Marking Accepted based on IRC approval-in-concept (depending on performance impact) from Russell and Jacob.

comment:2 Changed 4 years ago by carljm

  • Has patch set
  • Owner changed from nobody to carljm

Here's my initial go at a patch. It bases the sending of the signal on the value of TEMPLATE_DEBUG, and setup_test_environment forces the sending of the signal regardless.

As you can see, this patch does a boolean if check on every template render. This is the simplest, most straightforward code, so I decided to try it first and check the performance impact. I added a template_render_simple benchmark to djangobench (available at http://github.com/carljm/djangobench/tree/template_render_simple) to focus on the render call itself and eliminate as much noise as possible. Results also attached: djangobench certainly isn't detecting any noticeable slowdown; half the time it seems to think it's faster, which suggests to me that any impact is so small as to be completely insignificant.

If someone can detect a statistically significant slowdown from this change, or suggest any alternative benchmarking methods that might detect one, I have ideas for how the boolean check could be done just once at import time. But all of these ideas would involve less readable and maintainable code, so I'm going to vote for the simple approach in the absence of any evidence of a performance issue.

Changed 4 years ago by carljm

Changed 4 years ago by carljm

comment:3 Changed 4 years ago by carljm

comment:4 Changed 4 years ago by russellm

  • Needs documentation set

Patch looks good, except for the absence of docs. We're adding a new public signal, and an API that would allow people to subclass (or monkeypatch) Template to turn on that signal.

Actually, that makes me wonder if Template should maybe take a 'enable signals' argument so you can turn on signals on a per-instance basis, rather than needing to subclass or monkeypatch...

comment:5 Changed 4 years ago by carljm

  • Patch needs improvement set

Actually, on further review, this patch needs more work. I used a class attribute on Template to avoid a check every time a template is instantiated, but introducing an import-time settings dependency is not OK.

Given that there's already a TEMPLATE_DEBUG check in Template.init, I can probably piggyback on that with almost zero perf impact (I'll benchmark, of course), and having the API be via an arg to Template.init (defaulting to TEMPLATE_DEBUG) is nicer than a class attribute (which is basically a global). Just need to figure out how to allow the test system to force it to True...

comment:6 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:8 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from carljm 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.