It seems to me that automated tests are viewed by many developers as sort of a badge of “real programmer”-ness and as such are something to be written as a sense of duty if time permits. I think that this misses the real value of writing tests - they push you, if you chose to listen, towards writing simpler, more reusable code. If we take as a given that reusable and modular code is a useful goal (in all honestly I so seldom see reusable and modular code that I doubt this is a foregone conclusion) then writing tests will guide you towards that goal if you listen to what your tests are telling you.
For the examples here I’ll be using the kind of tests I normally see in django codebases as an example of poor tests. This is not a reflection on django itself; it’s just that as a predominantly python company that makes webapps we most often encounter django developers and so these are the anti-patterns we see most often.
For this example lets assume that we’re working on an application that does nothing more than display a randomised list of books. We’d typically expect to see something like this:
class TestBookView(TestCase):
def test_we_get_books(self):
book = Book.objects.create(title=u'Jack and Jill')
response = self.client.get('/books/')
returned_book = response.context['books'][0]
self.assertEqual(book.slug, returned_book.slug)The problems with this test are manifold but the most glaring to me is that it’s not at all clear what is actually being tested. It’s probably a view that is the intended unit under test but it’s entirely possible that the actual value the assertion is made against is based on a middleware, context_processor, template tag, logic in the template itself, the behaviour of Book.save (or a post_save signal) or something else I’m overlooking. Lets assume that it is the view that we’re testing here; we still have the issue of not being able to instantly determine exactly which view we’re testing. Sure, you just need to look up the view name in the urls module but as quick as looking up the view might be it would still surely be better if the test actually indicated what it was testing wouldn’t it?
There’s also the issue of time taken to execute the test. At the start of the project this test will be kind of fast to run but will only get slower as code is added to the application. The importance of speed for your tests is that it shortens the feedback loop between writing code and seeing the results of the test. Ever <alt>-<tab> to something else as some slow tests are running only to get distracted and not return to what you’re supposed to be doing for too long? I know that I have. Worse yet are those test suites that take so long to run that they are never run. As is, this test requires executing
middleware that is currently enabledcontext_processor currently enabled.Now imagine if this view was to become login protected the test code would have to be updated:
class TestBookView(TestCase):
def test_we_get_books(self):
admin = User.objects.create_superuser('admin', 'admin@example.com', 'password')
self.client.login('admin', 'password')
book = Book.objects.create(title=u'Jack and Jill')
response = self.cient.get('/books/')
returned_book = response.context['books'][0]
self.assertEqual(book.slug, returned_book.slug)Even though this test is purely to confirm that the view code is returning a list of books we now have to create a superuser and login. This kind of test churn is one of the things that turns people off of writing tests. Why should seemingly unrelated changes cause all of the tests to have to update?
An additional drawback with this form of test I first heard mentioned by JB Rainsberger is that each conditional statement introduces two possible code paths. In other words, for n conditionals there are n^2 paths. This means that for a function with 10 conditionals you’d need to write 100 tests to fully cover the possible execution paths. As it currently stands how many conditional statements do you think are encountered while passing through almost the entire django framework as this test currently does? I would guess a lot. Can you say with certainty that the application won’t add even more paths as it accrues features? It’s highly unlikely that it won’t. Wouldn’t be better to write tests that don’t need to execute all of those unrelated code paths and allow us to focus purely on covering the actual functionality that we’re interested in?
The first step to both clarifying and simplifying the test would be to make use of django’s RequestFactory which allows you to create an instance of a Request object you can pass to a view and therefore directly test just the view function that you’re interested in. RequestFactory shares the same API as the test client which means that you can POST and GET as you would with the test client:
class TestBookView(TestCase):
def test_we_get_books(self):
book = Book.objects.create(title=u'Jack and Jill')
factory = RequestFactory()
request = factory.get('/url-is-unimportant')
response = book_list_view(request)
returned_book = response.context['books'][0]
self.assertEqual(book.slug, returned_book.slug)Let’s also start to imagine the view that we’re actually testing:
def book_list_view(request):
books = Book.objects.order_by('?')
context = {'books': books}
return TemplateResponse(request, 'book_list.html', context)This is a much better test in that it doesn’t execute so much django code. The url resolution and middleware processing is all skipped here. TemplateResponse does use a request context and so the context_processor code is still executed however. If the view is updated to require login then the test code would need to be updated unfortunately:
@login_required
def book_list_view(request):
books = Book.objects.order_by('?')
context = {'books': books}
return TemplateResponse(request, 'book_list.html', context)
class TestBookView(TestCase):
def test_we_get_books(self):
book = Book.objects.create(title=u'Jack and Jill')
factory = RequestFactory()
request = factory.get('/url-is-unimportant')
request.session = BaseSession()
request.user = User()
response = book_list_view(request)
returned_book = response.context['books'][0]
self.assertEqual(book.slug, returned_book.slug)On the plus side, we have access to the request instance and so we can directly add session and user attributes (which is what the middleware would do) in order to satisfy the login_required decorator. Setting the request attributes in this way is quicker than creating a superuser in the db and calling the login view.
Now imagine a new requirement is added to allow a book to be “promoted” and always remain in the 1st five books returned based on a preferred subject of interest as defined in the current user’s profile. The first implementation’s view might look like this:
@login_required
def book_list_view(request):
# use a onetone relation to profile instead of user.get_profile()
subject = request.user.profile.preferred_subject
books = list(Book.objects.order_by('?'))
first_5 = books[:5]
if subject not in [book.subject for book in first_5]:
try:
promoted = [book for book in books[5:] if book.subject == subject][0]
except IndexError:
promoted = None
else:
promoted = books.pop(books.index(promoted))
if promoted:
insert_point = first_5.index(random.choice(first_5))
books.insert(insert_point, promoted)
context = {'books': books}
return TemplateResponse(request, 'book_list.html', context)Now the test needs to be updated to handle the profile which could look like this:
class TestBookView(TestCase):
def test_prefered_subject_in_first_5_books(self):
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Little red riding hood', subject='wolves'))
factory = RequestFactory()
request = factory.get('/url-is-unimportant')
request.session = BaseSession()
user = User.objects.create('alice', 'a@e.com', 'password')
request.user = user
UserProfile.objects.create(user=user, preferred_subject='wolves')
response = book_list_view(request)
first_5 = response.context['books'][:5]
self.assertTrue('wolves' in [b.subject for b in first_5])As you can see the amount of setup code needed to enable this test is growing. A common practice to avoid repeatedly having to perform the setup code is to either move this setup to the setUp method. If you’re really unlucky you might encounter a TestCase where the author has subclassed TestCase and created a specialised class that you can inherit from whenever code that exercises this view is needed. Another poor practice is to use a factory library of some sort to make it easier to create the 6 records needed to test with.
The pain that setting up the state to test this view causes is the most crucial benefit that writing tests can offer you and unfortunately so many developers misinterpret this pain and remove it in completely the wrong way. Whenever you encounter a situation like this where so much state is needed your test is telling you that the code you are testing is doing too much. When you step back to think about it, all we are trying to test is that there is always a Book instance with a certain subject in the first 5 items of the list. The critical piece of code has nothing to do with a logged in user and as we can see from the view code, although the preferred subject is derived from the user’s profile the actual profile the user profile itself has no bearing on the subsequent ordering of the books. Clearly none of the user, profile or session are actually needed to test the ordering of the books and so how can we make this view easier to test?:
def get_books(subject):
books = list(Book.objects.order_by('?'))
first_5 = books[:5]
if subject not in [book.subject for book in first_5]:
try:
promoted = [book for book in books[5:] if book.subject == subject][0]
except IndexError:
promoted = None
else:
promoted = books.pop(books.index(promoted))
if promoted:
insert_point = first_5.index(random.choice(first_5))
books.insert(insert_point, promoted)
return books
@login_required
def book_list_view(request):
# use a 121 relation to profile instead of user.get_profile()
subject = request.user.profile.preferred_subject
context = {'books': get_books(subject)}
return TemplateResponse(request, 'book_list.html', context)By extracting the logic that ensures the preferred subject is among the first 5 books we uncoupled it from the issue of logging a user in and getting their preferred subject from their profile. The code is now much simpler to test because we no longer need to create the request, the user, the profile or the session just to ensure that the preferred subject is in the 1st five items:
class TestBookView(TestCase):
def test_prefered_subject_in_first_5_books(self):
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Jack and Jill', subject='water'))
Book.objects.create(title=u'Little red riding hood', subject='wolves'))
books = get_books(subject='wolves')
first_5 = books[:5]
self.assertTrue('wolves' in [b.subject for b in first_5])We still have to create the six database records though so perhaps that factory might come in handy after all. Let’s pause for a second though and think if that’s really the case. We can see that get_books doesn’t actually make use of any of features of the queryset. In fact get_books only ever checks to see the value of book.subject. This means that we could actually rewrite get_books and the view to look like this:
def get_books(books, subject):
first_5 = books[:5]
if subject not in [book.subject for book in first_5]:
try:
promoted = [book for book in books[5:] if book.subject == subject][0]
except IndexError:
promoted = None
else:
promoted = books.pop(books.index(promoted))
if promoted:
insert_point = first_5.index(random.choice(first_5))
books.insert(insert_point, promoted)
return books
@login_required
def book_list_view(request):
# use a 121 relation to profile instead of user.get_profile()
subject = request.user.profile.preferred_subject
context = {'books': get_books(list(Book.objects.order_by('?')), subject)}
return TemplateResponse(request, 'book_list.html', context)(get_books no longer gets books and so should really be renamed but for now I’m leaving it as it is)
Writing to the database is one of the slowest operations that you can perform and so if it’s possible to avoid doing so then you really should. For the test we only need to instantiate the models instances; not save them:
class TestBookView(TestCase):
def test_prefered_subject_in_first_5_books(self):
books = [
Book(title=u'Jack and Jill', subject='water'))
Book(title=u'Jack and Jill', subject='water'))
Book(title=u'Jack and Jill', subject='water'))
Book(title=u'Jack and Jill', subject='water'))
Book(title=u'Jack and Jill', subject='water'))
Book(title=u'Little red riding hood', subject='wolves'))
]
books = get_books(books, subject='wolves')
first_5 = books[:5]
self.assertTrue('wolves' in [b.subject for b in first_5])Now at this point some may complain that we’ve modified the code purely to make it easier to test. Undeniably that is true. I would point out that it is equally true that we’ve modified the code to make it easier to reuse. By definition executing code during a test is reusing it and if, as stated earlier, we consider reusable code a desirable goal then by making the code testable we’re moving the code closer to our desired state. If it’s now possible to reuse this code during tests it must also be possible to reuse it elsewhere.
An interesting side-effect of these changes are that we’ve now stumbled into what is known as the Single Responsibility Principle (SRP). Briefly, the concept behind SRP is that each class, method or function should do one thing only. When we first extracted get_books it was doing two things: getting a set of records from the database and modifying the order of those records in memory based on some requirement. As we modified get_books to make it more reusable we fell in line with SRP. Now of course SRP along with any other programming guidelines is not an inviolable rule but it is something that often leads to reusable code.
A second useful programming concept we stumbled into is Dependency Injection (DI) which basically is a technique wherein hard-coded dependencies are removed from the code body and are instead passed in to the function or method as arguments. In our get_books function we removed the reference to Book.objects.random and instead just passed in an iterable because all the ordering of books really needs is an iterable of book-like items. For our purposes it was useful to not have to store the records in the database and just pass in the iterable but what has happened now is that get_books has opened up for re-use in ways we may not have imagined. If perhaps the query needed to get the initial list of books needs to change that will not affect get_books. In fact because get_books has no knowledge of what it’s sorting it can now be used so sort any iterable of objects with a subject attribute in the same way. This makes use of duck-typing and polymorphism.
As an example, imagine that we start incorporating an additional source of books as provided by a 3rd party that returns JSON. We could easily pass this new source to get_books if we thinly wrap the JSON to provide the correct attribute.
Now that get_books neither gets books nor even does anything in particularly book-related lets rename it to reorder_items (not an amazing name but naming is one of the two hard problems of computer science after all):
class Booklike(object):
def __init__(self, obj):
self.original_json_object = obj
@property
def subject(self):
return self.original_json_object['topic']
@login_required
def book_list_view(request):
subject = request.user.profile.preferred_subject
books = [Booklike(obj) for obj in get_books_as_json()]
context = {'books': reorder_items(books)}
return TemplateResponse(request, 'book_list.html', context)Now at this point I suspect that some may argue that You Ain’t Gonna Need It makes tight coupling a valid approach and perhaps they are right. I would counter that YAGNI applies more to functionality than design. The changes our tests led us towards don’t increase the functionality that the application provides; they merely led us towards providing that same functionality in a better designed way. That’s why tests are useful.