Why you should not use SetUp and TearDown in NUnit

When I lead the team that built NUnit we implemented SetUp and TearDown similar to the way they were implemented in JUnit. For those who do not know SetUp and TearDown are attributes on methods in a TestFixture that perform common initialization and destruction.  Here is an example that demonstrates how and when SetUp and TearDown are called,

[TestFixture]
public class TestFixtureLifetime
{
    [SetUp]
    public void BeforeTest()
    { Console.WriteLine("BeforeTest"); }

    [TearDown]
    public void AfterTest()
    { Console.WriteLine("AfterTest"); }

    [Test]
    public void Test1()
    { Console.WriteLine("Test1"); }

    [Test]
    public void Test2()
    { Console.WriteLine("Test2"); }
}

When I run this test  I get the following output: .

BeforeTest
Test1
AfterTest
BeforeTest
Test2
AfterTest

I used to think that factoring out duplicated code into a common SetUp and TearDown method was good idea. My reasoning was based on one of the tenants of Simple Design; Remove Code Duplication, even from your test code. In order to better demonstrate why my thoughts have changed let's look at a slightly modified MoneyTest from the NUnit samples. MoneyTest has 3 Test methods (IsZero, BagAdd, and BagSubtractIsZero) and a Setup method, (BeforeTest). 

[TestFixture]
public class MoneyTest
{
    private Money f12CHF;
    private Money f14CHF;
    private Money f7USD;

    private MoneyBag moneyBag;

    [SetUp]
    public void BeforeTest()
    {
        f12CHF = new Money(12, "CHF");
        f14CHF = new Money(14, "CHF");
        f7USD = new Money(7, "USD");

        moneyBag = new MoneyBag(f12CHF, f7USD);
    }

    [Test]
    public void IsZero()
    {
        Money[] bag = { new Money(0, "CHF"), new Money(0, "USD") };
        Assert.IsTrue(new MoneyBag(bag).IsZero);
    }

    [Test]
    public void BagAdd()
    {
        Money[] bag = { new Money(26, "CHF"), new Money(7, "USD") };
        MoneyBag expected = new MoneyBag(bag);
        Assert.AreEqual(expected, f14CHF.Add(moneyBag));
    }

    [Test]
    public void BagSubtractIsZero()
    {
        Assert.IsTrue(moneyBag.Subtract(moneyBag).IsZero);       
    }
}

The problem that I have with SetUp in this case is twofold. The first and primary complaint is that when I am reading each test I have to glance up to BeforeTest() to see the values that are being used in the test. Worse yet if there was a TearDown method I would need to look in 3 methods. The second issue is that BeforeTest() initializes member variables for all 3 tests which complicates BeforeTest() and makes it violate the single responsibility pattern.  In order to fix this I would remove the BeforeTest() method and place the specific variables in each test - here is the refactored code:

[TestFixture]
public class NoSetUpMoneyTest
{
    [Test]
    public void IsZero()
    {
        Money[] bag = { new Money(0, "CHF"),
                        new Money(0, "USD") };
        Assert.IsTrue(new MoneyBag(bag).IsZero);
    }

    [Test]
    public void BagAdd()
    {
        Money f7USD = new Money(7, "USD");
        Money f12CHF = new Money(12, "CHF");
        Money f14CHF = new Money(14, "CHF");
        Money f26CHF = new Money(26, "CHF");

        Money[] bag = { f26CHF, f7USD };
        MoneyBag expected = new MoneyBag(bag);

        MoneyBag moneyBag = new MoneyBag(f12CHF, f7USD);

        Assert.AreEqual(expected, f14CHF.Add(moneyBag));
    }

    [Test]
    public void BagSubtractIsZero()
    {
        MoneyBag moneyBag = new MoneyBag(new Money(7, "USD"),
                                         new Money(12, "CHF"));
        Assert.IsTrue(moneyBag.Subtract(moneyBag).IsZero);
    }
}

In the refactored code the BeforeTest() is removed and each test is forced to do the initialization for what it needs to run. This makes it very easy to read each test as a single entity. Also, it has the side benefit of making it very clear which tests are more complicated than others. Compare the initialization code needed for BagAdd compared to IsZero. In addition the new class has no member variables. This is critical for test isolation. Without member variables there is no mechanism contained within the class for one test to mess with another.

These reasons lead me to never want to use common setup and tear down methods. In fact, I believe that this feature should be removed from the tool. Some people will argue that you cannot build a tool that stops people from writing poor code. However, you can write a tool that prevents people from shooting themselves in the foot.

Comments

[Kartones]

While I agree with you that it can be very dangerous and change the behavior if wrongly used, they are quite useful too if you use them right.

I usually employ a mixed tactic:
-Some "used almost everywhere" variables and objects initialized in the Setup. This objects are never used in "basic" tests (example below).
-Every other variable in it's test.

This way, if I have a TestUserNameIsNotNull, I create another User object to see if it's property Name is null or not. I don't use testUser1 (which would be instantiated inside Setup).

My aproach requires more thinking, yours is 100% safer as all "constructor/destructor code" has to be explicity written/added.

[Scott Bellware]

Great food for thought! I think about dropping setup and teardown often. Haven't come to the same firm conclusion that you have, but I haven't had as much time on-task as you have had and I don't have the benefit of the extent of your experience.

I do think that the initialization of test concerns in setup that don't pertain to all tests in a test class is an anti-pattern. I avoid it when possible, and when it's not possible, I tend to think that the design of the test class is suspect.

Using RSpec has changed the way I think about the size of NUnit test classes. I'm more prone to using smaller, more focused classes that reflect specific contexts rather than continuing to use the one-master-test-class-per-functional-class pattern.

Maybe it's a bit ironic, but I find that setup ("before" blocks in RSpec) and teardown ("after" blocks) are more acceptable when I use these small, focused test context classes.

In fact, I sometimes even use them in a context that has only one test method. I do this to call out the difference between the code that represents the actual test concern and any setup (or teardown) code.

In very small test context doses, I find that setup and teardown blocks can be effective.

[Jay Fields]

I completely agree. I wrote something similar, with examples in Ruby. http://blog.jayfields.com/2007/06/testing-inline-setup.html

[Thomas Eyde]

I rarely use TearDown. When I do, it's usually to clean up sideeffects of my tests, like deleting files or table rows. Those things shouldn't have anything to do with the actual thing being tested.

I would treat most of your variables as constants, and move their initializing up to setup or field declaration. I would also rename them to usd7, chf14 and so on. These variable names shouldn't need a lookup.

Finally, I don't like to clutter my tests with unreadable initialization code. So when the initialization becomes cluttered, I try to make the objects easier to initialize, extract the code to methods, or both.

In this case, while not needed here, one of them could be:

Money[] bag = CreateBagFilledWithEmptyMoney();

[Mladen]

I usually set the stuff i need to use in all tests in the class constructor.
so i like the setup and teardown methodology.

[Brig Lamoreaux]

I can see in this situation how Setup() and TearDown() complicate the testing. However, I'm in a situation where Setup() dramatically reduces the amount of code and simplifies the entire testing tremendously. I use NUnit and WatiN to test web applications. Each TestFixture is testing a specific page and the Setup() method contains the code to login with specific user and navigate to the page. For now, I can't do away with Setup() but I can get rid of TearDown() because I rarely use it.

[Tudor-Andrei Pamula]

Usually, I build the "setup" in the test methods, and only after that I refactor it in the setup. In such a situation I get a better visibility: this is a "general" setup, this is a special setup, I could refactor this to a factory method, I have several setup-factory methods which I can combine together.

Or I get to a very different conclusion, that I test 2 very different things, and I should create a new test fixture.

I would not exclude Setup/TearDown. They "bite" you, only when you define up-front what are they. In a "pure" TDD way, you wouldn't do that: just small-test, refactor, small-test, refactor...

my 2 cents...

[Peter Hancock]

To me, the money bag test looks like a DoEverything test class, and falls under the "one fixture per class" design anti-pattern. I call them tightly coupled tests and wrote about it just recently - http://www.bottleit.com.au/blog/post/Loosely-couple-your-tests-to-your-implementation.aspx

I would factor the tests into significantly smaller units, something that Scott Bellware is calling a context here --http://codebetter.com/blogs/scott.bellware/archive/2007/09/21/168390.aspx
(Namespace.Fixture.Test)
MoneyBag.InitializationDefault.TotalIsZero
MoneyBag.EmptyAdd.SingleCurrency
MoneyBag.EmptyAdd.MultipleCurrency
MoneyBag.NotEmptyAdd.SingleCurrency
MoneyBag.NotEmptyAdd.MultipleCurrency
MoneyBag.NotEmptySubtract.SingleCurrency
MoneyBag.NotEmptySubtract.MultipleCurrency
MoneyBag.NotEmptySubtract.MoreThanItContainsThrows

You keep your setups to populate JUST for that test context. You refactor moneybag creation into a factory method. Chances are that as you realise you're constantly creating money bags in code, you end up promoting the factory method into a first class citizen and using it in not just the test, but the production code also. Your test has effectively driven your design.

[Peter Hancock]

Damn, snipped the bottom of my post off... I went on to state...

The advantage now is that your setup method becomes purely a container for preparing the tests for specifically those cases, and you're not polluting the actual test logic itself with non-testing related code. In other words, the test contains the bare minimum amount of code required to ascertain whether the test passes or fails.

I'm with you though on removing [ExpectedException]. (In your post about xunit features) It's odd in NUnit because the test pass/fail logic is moved out of the test method itself and into an attribute. It's not metadata about a test at all - it IS the test - so it belongs as an assert.

[J. B. Rainsberger]

I'm not sure it's wise to use a bad example of fixture objects to illustrate why fixtures are a bad idea. The values in the fixture are used haphazardly in the tests, so they're certainly better off as local variables in each test.

That said, I wish I'd known when I first read the Money example that it was using fixture objects poorly. That's not the fault of set up or tear down, but rather of the quality of the example.

When testing a stateless service, is it really such a good idea to instantiate the same service object the same way at the beginning of 8 different tests? Do you honestly believe that's better than instantiating it once in a set up method? That would surprise me.