setUp in unit tests: mostly harmful

7 April 2011 at 11:10 4 comments

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'}))
Advertisements

Entry filed under: Launchpad, Programming. Tags: , .

Ubuntu is the readers’ choice New work – bigger challenges – great stuff

4 Comments Add your own

  • 1. Jonathan Lange  |  7 April 2011 at 11:58

    I largely agree. It leads to fragile fixtures (not all of the tests need all of the setup) and obscure tests because of the mystery guests.

    You might be interested in these patterns:
    * http://xunitpatterns.com/Implicit%20Setup.html
    * http://xunitpatterns.com/Inline%20Setup.html
    * http://xunitpatterns.com/Obscure%20Test.html#Mystery Guest

    Or the book, http://xunitpatterns.com/, which is excellent.

    jml

    Reply
  • 2. Danilo  |  11 April 2011 at 09:26

    One good characteristic of this “anti-pattern” (as you call it) is that it may “force” you to create a new test case for an entirely unrelated test case. I’ve seen too many (unrelated) tests dumped together into a single TestCase class simply because people are lazy to bother with boiler plate.

    Not saying that’s the case here, but noting that as far as “good habits” go, this one has something in it as well.

    Reply
    • 3. Jonathan Lange  |  13 April 2011 at 08:18

      Why does it matter if lots of tests are in a test case class?

      I find that the opposite tends to happen more: tests that aren’t very related get lumped together because they have similar setUp.

      Reply
      • 4. Henning  |  13 April 2011 at 08:28

        Yes, I agree. Usually stuff accumulates in setUp, a lot of which is only used by a fraction of the tests. If setUp is not being used, it does not really matter which test case class a test is in as each test is self-contained.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Trackback this post  |  Subscribe to the comments via RSS Feed


Ubuntu 11.10 days to go
Banner by picomol.de

%d bloggers like this: