setUp in unit tests: mostly harmful

While hacking on my current Launchpad bug today an anti pattern hit me again that I have been fighting for some time. Everybody knows that sample data should not be used in tests and anybody earns bonus points for removing such use from tests in the Launchpad source tree. Much less known is that the setUp method is overrated and should not be used to create sample data for a test.

This is an anti-pattern that I have stopped using some time ago but I still come across a lot in the Launchpad source code. Sample data is created in setUp as instance variables of the test case, like this one here in my current example:

def setUp(self):
    super(TestPOExportView, self).setUp()
    self.pofile = self.factory.makePOFile()
    self.potemplate = self.pofile.potemplate
    # All exports can be requested by an unprivileged user.
    self.translator = self.factory.makePerson()

Then, this sample data is used in other helper methods and the tests itself.

def test_request_format_po(self):
    # It is possible to request an export in thePO format.
    self._createView({'format': 'PO'})

    self.assertContentEqual(
    [(self.potemplate, self.pofile, TranslationFileFormat.PO)],
        get_poexportrequests(include_format=True))

This is bad for two reasons: degraded readability and lack of extensibility.

To the casual reader of this test it will not be clear, what kind of view is being created and how self.potemplate and self.pofile relate to each other and to the test. Granted, in this case it is not very hard to guess but I have seen more complicated sample data creation in setUp.

Also, when extending the test it is not possible to use _createView with a different type of POFile without touching setUp and thereby risking to break existing tests. This is actually what caused me trouble here.

I removed setUp from this test completely. The translator person was only needed when creating the view, so its creation was simply moved into _makeView. I added a parameter to _makeView to take the POFile which to create the test for. The test now looks like this:

def test_request_format_po(self):
    # It is possible to request an export in the
    # PO format.
    pofile = self.factory.makePOFile()
    self._createView(pofile, {'format': 'PO'})

    self.assertContentEqual(
      [(pofile.potemplate, pofile, TranslationFileFormat.PO)],
      get_poexportrequests(include_format=True))

You can see how much clearer this is. I admit that the setUp was so simple that in this case it could easily be replaced with a single factory call. More complex sample data call for specialized factory methods in the test case that take numerous parameters to set up the sample data in the way it is needed for the test. I usually call these methods _makeSomethingAndSomethingElse or _makeSomethingWithSomethingElse. An example can be found in the test case at hand.

The other reason for avoiding this anti-pattern is to maintain extensibility. With the pofile object not being implicitly created by setUp and used by _makeView I can now create a pofile with different parameters and pass it to the test.

def test_pochanged_option_available_ubuntu_no_upstream(self):
    # The option for partial exports is always available on a
    # source package, even if there is no upstream pofile.
    ubuntu_pofile = self.factory.makePOFile(
        side=TranslationSide.UBUNTU)
    view = self._createView(ubuntu_pofile)

    self.assertTrue(view.has_pochanged_option)

That’s all I really wanted to do when I touched this test case but the anti-pattern of using setUp to create sample data forced me to refactor the whole test case. Please be considerate of the developers coming after you and avoid this pattern. Thank you. (Did I mention that the guy who wrote this test case in the first place looks a lot like me? ;-))

Oh, and there is good use for setUp! It is the perfect place to set feature flags, like so:

def setUp(self):
    super(SomeTestCase, self).setUp()
    self.useFixture(FeatureFixture(
        {'translations.sharing_information.enabled': 'on'}))