This is the fourth in a series of guest posts by Andrew Tate, a writer turned software developer at Animalz. He has been using PullRequest code review to help improve their first product, Revive, and learn more about development.

1. 3 Lessons Learned from My First Week of Code Review
2. Learning Security One Code Review at a Time
3. Solving Problems Collaboratively Through Code Review
4. Challenge Your Testing Fears With the Help of Code Review

When I first started building Revive, I talked to an experienced engineer about getting started on my first project.

“Write tests,” he said. Of course, I didn’t do this.

Looking at that infographic, it seems like I’m not alone either. For all of the emphasis on test-driven development, the Stack Overflow Developer Survey 2019 reports that more than one-third of developers don’t test their code (with almost 5% replying,“No, and I’m glad we don’t”). Only about 40% have a process in place to write unit tests. Far more people see the importance of code review than see the need for testing.

I can definitely understand the hesitancy. Testing doesn’t feel like you are moving the ball forward. Particularly when you are a lone developer trying to get something out, tests seem superfluous—coding for coding’s sake.

But auditing my entire codebase highlighted a problem with this thinking. From the main reviewer:

. . . it looks straightforward to extend and automate when the time is right. A key step in this direction will be building out an automated test suite — currently, the code appears to be untested, which will make it more and more difficult to build on top of as time goes on (and particularly as the size of your engineering team grows).

I am doing exactly that right now. I am building out this product with more functionality. As I do, I find functions that have unclear behavior, and I’m not sure what will happen if I change them. Will I break something integral to the existing app?

Without tests, it will become “more and more difficult to build on top of as time goes on.” But having never written a test before, I have no idea whether my tests will test correctly. This is an ideal case for code review. With so many people valuing code review in their projects, as reported in the same Stack Overflow Developer Survey, this seems like the perfect opportunity to try out both together—a code review for setting up my first test suite! Though everyone says you should begin with tests, it isn’t a beginner subject, so having experienced reviewers walk me through the nuts and bolts of testing is exactly what I need.

First test, first fail

To understand the layout of Django projects, I was using a previous project another experienced developer had written.

His tests.py file read (and still reads) just:

# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.test import TestCase

# Create your tests here.

Clearly, there’s not a lot to learn from. This is one of the benefits of code review in this scenario. By leaning on the reviewers, I can find mistakes in my tests that are finding mistakes in my code.

My first test was the simplest I could conceive. Test whether the user model was storing data correctly:

from .models import User

pytestmark = pytest.mark.django_db

class TestUserModel:

    def test_save():
        email = 'andrew@animalz.co'
        datetime_now = datetime.now()
        unique_id = uuid.uuid4()
        user = User.objects.create(
            email=email,
            created_at=datetime_now,
            unique_id=unique_id)

        assert user.email == email
        assert user.created_at == datetime_now
        assert user.unique_id == unique_id 

As before, this was a test case I had copied from a tutorial. But when I ran it, it immediately failed:

FAILED refresh/tests.py::TestUserModel::test_save
 - assert datetime.datetime(2020, 4, 15, 10, 7, 45, 632818) 
 == datetime.datetime(2020, 4, 15, 10, 7, 45, 632220)

The time is different. Although I used the same time as the input to the model and in the test assertion, the way I had set up this model in Django meant it overrode by datetime_now variable with the exact time, a few milliseconds later, that the user object was created.

This left me stumped. If the time changes on the fly, as it invariably does, how can I assert later that the input and output times are the same and test this code? Code review comes to the rescue.

I recommend using freezegun here instead. You can see more about this package here: https://github.com/spulec/freezegun It will ensure that you are able to get deterministic outputs from date time functions. Note that in some cases the date time computation might be done on the SQL server side, which would then not get mocked out of course.

— Code reviewer Olivier LF.

Using freezegun, I was able to freeze the time and generate my first test pass!

from freezegun import freeze_time

from .models import User

pytestmark = pytest.mark.django_db

class TestUserModel:
        
    @freeze_time("2011-11-11")
    def test_save(self):
    email = 'andrew@animalz.co'
    unique_id = uuid.uuid4()

    user = User.objects.create(
            email=email,
            unique_id=unique_id)

    assert user.email == email
    assert user.unique_id == unique_id
    assert user.created_at == datetime.now()

In this case, the review didn’t stop just at this suggestion. I also got two critical extra pieces of information regarding tests from different reviewers.

First:

You should likely reload the model from the database before testing these values: https://docs.djangoproject.com/en/3.0/ref/models/instances/#django.db.models.Model.refresh_from_db

—Code reviewer Olivier LF.

This points out my rookie mistake. My test isn’t testing what I think it is testing. I think it is testing saving the object to the database, but if I’m not refreshing it from the database, I can’t be sure it has saved correctly.

Second:

In general, automated testing needs to be deterministic, quick, and relatively small in scope. Using datetime.now() and auto_now_add understandably would make things much harder to test. as you brought up, it is better to mock/monkey patch these values out or to use hardcoded dummy values directly in your tests instead so it’s easier to test all code-paths and makes it deterministic.

—Code reviewer Proctor R.

This gave another way to deal with the time issue. But this comment also provides two high-level pieces of advice for testing. One, that tests need “to be deterministic, quick, and relatively small in scope.” This gives me a rubric for writing future tests. Two, “[I]t is better to mock/monkey patch these values out or to use hardcoded dummy values directly in your tests.” This was another philosophical issue I have been grappling with when understanding tests—can I mock data? Yes! This feedback allows me to worry less about the data going into the function and more about what exactly I am testing.

This was a lot to learn from such a simple first test. But having reviewers who were willing to walk me through the basics and answer my initial questions meant that I was able to learn from their experience and iterate quickly. Rather than testing slowing me down, testing + code review allows for better quality code at a high speed.

Testing as a new way of thinking, code review as a way to test that thinking

This has been one of the most useful code review sessions I have had so far. It has introduced me to a new way of thinking about my code.

My first view of tests was that I didn’t need any tests, since I thought I didn’t have any time to make and maintain them. The audit convinced me otherwise, with so many glaring bugs that could have been quickly caught with simple unit tests. But, even then, I was expecting to think about tests only in terms of bug fixes and reactionary testing as suggested in the audit comments:

For now I’d recommend trying to write tests when you find logical errors in your code. Every time you fix a bug, try to add a test to verify the bug is fixed. If there are parts of the code that have been errorprone in the past, you might consider going back and adding tests for those as well. Take a reactionary approach to testing (“what tests could we have had in place to catch this issue earlier?") rather than striving to preemptively catch every issue.

—Code reviewer Chris E.

This is how I got started, but already with these few tests and the feedback from review, I am seeing them in a different light—thinking about them more deeply and about the exact problem I am solving with each one. This feedback concerning a couple of tests I had written for a small CUSUM function highlights this well:

Testing calculate_cusum on a couple of known outputs, as well as on potential edge cases (empty list, or other uncommon, but plausible for your system, inputs) will be valuable to more easily understand where issues are in the code when they arise as well as to increase confidence in future refactorings working as intended.

—Code reviewer Olivier LF.

What are the known inputs to this function? What will the edge cases be? In what case would I pass an empty list to this function, and what do I expect it to return? These are all small concepts, but they are tangible questions to ask about the code, and ones that make you think not just about the function but also the way data is moving from function to function.

Now I understand the motivation behind test-driven development. Write the tests first to describe how the function will work and, more importantly, help you understand how the function will work. Then, writing the function itself is trivial. Without the advice from the reviewers, I would not have reached this insight. And I wouldn’t have any working tests either.

Conclusion

Tests and code review work together. Tests are an important part of code and an important part of the review process. Like getting started with code review, getting started with tests requires good feedback to guide the way. You have to understand whether you are going down the right path.

If you get good help, like I have here, you don’t get just the straightforward review of your code; you also get guidance to continue learning away from the review and the knowledge that people are willing to help you get better:

Don’t hesitate to clarify the issue encountered with the beautifulsoup4 tests, in case we can assist. Please reply inline if you have any questions. I’ve added some thoughts on testing the getSoup method including links out to resources on mocking, patching, and some packages you can use to avoid reimplementing a lot of boilerplate when dealing with patching and the requests module.

—Code reviewer Olivier LF.

Beyond that, you also get greater insight into how your code works, how code works, and how to think about code. You come away not just with better code but with a better way to write all your code in the future as well.