Last time, I began fixing a fairly simply space-within-anchor-tags bug in djangocms-picture.
I’m using this exercise as an opportunity to (a) try out a disciplined development process in the context of a Django application and (b) dig into the Django and Django CMS APIs.
This involves writing a failing test against the existing plugin rendering code, seeing it fail, fixing the bug, and then seeing the test pass.
My experience so far with this framework has not been as stellar as I had originally thought it would be.
Two and a half months ago, I said, “I’m falling in love with Django.” Well, the infatuation has worn off. (Gee, that was fast.) In fact, I’m now thinking in terms of a longer-term website revamp plan using Flask on the public-facing web server (although I still might use Django to build a private-facing admin UI). That’s a different post.
This post is about going through the bugfix process with djangocms-picture.
One might ask why I need to go through the whole process for such a simple change. After all, I’m just removing some errant whitespace from an HTML template.
I think it’s poor practice to implement code changes, even simple ones, without proper automated testing. If it’s so simple to implement, it ought to be just as simple to test. If it isn’t, that’s a testing process smell. (Gerard Meszaros goes into such ideas in quite some detail in his excellent book xUnit Test Patterns.) Therefore, the experience tells me something I need to know about Django and Django CMS.
I do concur that it’s not necessary to unit test trivial code (i.e., no control flow statements and no data dependencies between statements, e.g., a series of simple attribute assignments, or a simple method delegation) that will be exercised by other tests. (That’s three separate qualifiers.)
However, this fix is not of that sort. It requires a change in the plugin template, which has numerous conditional blocks. And rendering the plugin HTML using Divio’s recommended test template also exercises the plugin’s render() method, which itself has control flow statements.
That standard test template also depends on the plugin model class. It’s an integration test, not a unit test.
At this point, I need to choose whether to go off on a tangent to design a unit-test template for this and to distinguish unit tests from integration tests. It might require mocking the model class, and it wouldn’t gain me anything in terms of test coverage (although that’s not all that counts). Furthermore, with each new tangent, I’m heading further and further from Django CMS’s standard practice. That in itself tells me something I need to know about Django CMS. As I said, the infatuation wore off pretty fast.
The Basic Test
Divio shows a sample Django-CMS-plugin test method that renders the plugin’s HTML thusly:
def test_plugin_html(self):
placeholder = Placeholder.objects.create(slot='test')
model_instance = add_plugin(
placeholder,
MyPlugin,
'en',
)
renderer = ContentRenderer(request=RequestFactory())
html = renderer.render_plugin(model_instance, {})
self.assertEqual(html, '<strong>Test</strong>')
I refactored that right off the bat, by creating the renderer object in the setUp() method. It’s part of the fixture and doesn’t need to be cluttering up the test code.
(You can see my code on GitHub.)
The test also needs to set attributes of the model, like external_picture and link_url. It took some delving into the Django CMS source to determine that I can pass them to add_plugin(), like so:
instance = add_plugin(
placeholder,
PicturePlugin,
'en',
**data
)
I put that code, as well as the code that creates placeholder, into a parameterized setup method. We don’t need that cluttering up the test, either.
There’s a namespace clash here. The signature of add_plugin() is as follows:
def add_plugin(placeholder, plugin_type, language, position='last-child',
target=None, **data):
The parameters packed into data are fed directly into the plugin model constructor. So you’d better hope that your data model doesn’t have any attributes named placeholder, plugin_type, language, position, or target, or else you’ll need to set those attributes some other way. This namespace clash could’ve been avoided simply by passing in data as a dictionary instead of as unbound parameters. After all, data is more aptly named model_initializers, as it is associated with initializing the model, not adding a plugin. None of this requires an advanced degree in aerospace engineering to figure out. It only requires a tiny bit of forethought, and maybe a code review. SMH.
I guess sometimes there’s only one best wrong way to do it, too.
Can we finally run some code yet?
Yes, I think so.
Rather than a bunch of test methods that all say the same thing with different data, I ended up creating a table-driven test that does the following:
for case in cases:
instance = self._create_instance(**case['data'])
html = self.renderer.render_plugin(instance, {})
self.assertHTMLEqual(html, case['expected_html'], case['name'])
That’s setup, test, and verify for each scenario being tested. (No explicit teardown is required.) This is the standard four-phase test structure.
I created cases for two scenarios (one at a time, each time commenting out pieces of the template to see the test fail, before restoring the template and seeing it pass—that’s all SOP):
cases = [
{
'name': 'external_picture',
'data': {
'external_picture': image_url,
},
'expected_html': '<img src="%s"/>' % image_url,
},
{
'name': 'external_picture with link_url',
'data': {
'link_url': link_url,
'external_picture': image_url,
},
'expected_html': '<a href="%s"><img src="%s"/></a>' % (link_url, image_url),
},
]
Why doesn’t it fail?
Running this test ought to fail.
The generated HTML doesn’t match the expected HTML. The generated HTML has semantically significant whitespace inside and around the anchor tag. The expected HTML has no whitespace.
I knew it was going to misbehave, because I had read the documentation for assertHTMLEqual(), which says, “Whitespace before and after HTML tags is ignored.”
What the flying… ?!
That’s actually not true, either. As it turns out, it only ignores whitespace between tags if there’s no other text. Here’s how I found out.
I sifted through the code behind assertHTMLEqual(), including a 106-line while loop in CPython’s html.parser, which contains 4-level-deep if statements, no blank lines, non-descriptive variable names, and nonorthogonal abstractions, but at least it includes the comment # end while sandwiched between two if blocks.
Maybe all it takes to stop me swooning over the beauty of Python is to actually look at what passes for production Python code. It’s sometimes no better than 1990’s-era Perl code. Really.
Then I finally found what I was looking for, in django.test.html.Element.append().
Sometimes I really wonder what developers are thinking.
# removing last children if it is only whitespace
# this can result in incorrect dom representations since
# whitespace between inline tags like <span> is significant
if isinstance(self.children[-1], str):
if self.children[-1].isspace():
self.children.pop()
Hint: if you have to document how the code you just wrote is incorrect, then you should probably not write it. Also, comments should explain why you did something that doesn’t make sense, not what you did. I could find no explanation as to why this was done, not in the commit message and not in the original ticket. My guess is that it was an attempt to do something with HTML’s spacing rules, probably to make some extant test work.
I have not reached out to the original developer for comment.
This was a poor design decision. We can safely ignore leading and trailing whitespace. But aside from that, short of rendering the HTML (or providing an optional-whitespace code in the expected text), it would have been better to require all whitespace to be documented in the test, whether semantically significant or not. This would make the tests slightly more fragile but would also avoid type II errors–which is the whole point of tests.
Hacking around this bullshit will have to wait for part trois (which is two more posts than this series ought to have required).
Still typing…
And may all your bars turn from red to— Aw, who gives a shit?
