views:

166

answers:

2

I'm testing the delete method of an abstract base repository class which is inherited by several other repository classes. MyTestRepository inherits the base so I can run tests against the base's methods without restricting my test to using a concrete class. When I run my unit test it passes, but I noticed afterwards I have several OrderDetail and Schedule objects in the test DB that were generated by the test (objects get created during test initialization) and not deleted, while the Order object is deleted. I added some breakpoints and noticed that as soon as the helper method ends and the expected exception has been thrown the test ends and the other calls to the helper never occur.

This is my first attempt at unit testing. Are my methodologies wrong? Is ExpectedException working as intended and I'm misusing it or is there another tool I should be using? The only way I can think of to get my test is to put a try catch block in the helper and assert true when I catch my DataAccessException.

    [TestMethod]
    [ExpectedException(typeof(DataAccessException))]
    public void NHibernateRepositoryBaseDelete()
    {
        NHibernateRepositoryBaseDeleteHelper<Order, int>(myOrder, myOrder.OrderId);
        NHibernateRepositoryBaseDeleteHelper<OrderDetail, int>(myOrderDetail, myOrderDetail.OrderDetailId);
        NHibernateRepositoryBaseDeleteHelper<Schedule, int>(mySchedule, mySchedule.ScheduleId);
    }

    private static void NHibernateRepositoryBaseDeleteHelper<T, TKey>(T myItem, TKey myItemId)
    {
        MyTestRepository<T, TKey> myRepository = new MyTestRepository<T, TKey>();
        myRepository.Delete(myItem);
        myRepository.CommitChanges();

        myRepository.GetById(myItemId, false);
    }
+5  A: 

I don't generally use ExpectedException unless I can cause the exception to be thrown in a single statement - or if other tests ensure that the earlier statements don't throw the exception.

Here, you've basically got three tests - you're testing that each of those delete calls will throw an exception. All that ExpectedException does is run the method and check that it threw the exception you asked it to - it doesn't try to continue from where the exception was thrown, expecting it to be thrown again.

If you want to check that an exception is thrown by a specific piece of code (rather than just the whole method) use:

try
{
    OperationThatShouldFail();
    Assert.Fail("Expected exception");
}
catch (DataAccessException)
{
    // Expected (no need for an assertion though)
}

(And don't have ExpectedException any more - you no longer expect the test method to throw.)

You'd have one of these blocks for each of the three checks. Alternatively (and probably better) just have three tests, each of which uses ExpectedException but is only one line long. As another alternative, you could put the try/catch into your helper method.

You might also want to have an assertion at the end of the test that the relevant tables are empty - but that depends on your situation.

EDIT: As for when you clean up the database - I generally like to clean it at the start of each test so that if I run just a single failing test I can see the state of the database afterwards. If I were to clean it up in the teardown method, I'd have lost valuable information (or be forced to stay in the debugger).

EDIT: Another alternative to ExpectedException (which I suspect is in many test frameworks now) is to have a generic method like this:

static void ExpectException<T>(Action action) 
    where T : Exception
{
    try
    {
        action();
        Assert.Fail("Expected exception " + typeof(T));
    }
    catch (T)
    {
        // Expected
    }
}

You can then call it easily (multiple times) from within the method using a lambda expression for the action, assuming you're using C# 3. For example:

// Method name shortened for simplicity, and I'm assuming that type inference
// will work too.
public void NHibernateRepositoryBaseDelete()
{
    ExpectException<DataAccessException>(() => 
        DeleteHelper(myOrder, myOrder.OrderId));
    ExpectException<DataAccessException>(() => 
       DeleteHelper(myOrderDetail, myOrderDetail.OrderDetailId));
    ExpectException<DataAccessException>(() => 
       DeleteHelper(mySchedule, mySchedule.ScheduleId));
}
Jon Skeet
Using ExpectedException works well for me, though admittedly it works best for short tests. I find it makes the test very readable; I can see what the test does at a glance.
Robert Lewis
Well, you can see a single overall desired result at a glance - but you can't tell which line is going to throw the exception. It's a test which can easily pass even if the code is broken, just because an exception is thrown earlier.
Jon Skeet
+1  A: 

Wrap your unit test code in a try/finally block and clean up your database inside the finally part.

[TestMethod]    
[ExpectedException(typeof(DataAccessException))]    
public void NHibernateRepositoryBaseDelete()    
{
    try
    {
        NHibernateRepositoryBaseDeleteHelper<Order, int>(myOrder, myOrder.OrderId);
        NHibernateRepositoryBaseDeleteHelper<OrderDetail, int>(myOrderDetail, myOrderDetail.OrderDetailId);
        NHibernateRepositoryBaseDeleteHelper<Schedule, int>(mySchedule, mySchedule.ScheduleId);
    }
    finally
    {
        // clean up database here
    }   
}
Robert Lewis
better than my answer
Preet Sangha
No, because that doesn't test the second and third lines calls to NHibernateRepositoryBaseDeleteHelper.
Jon Skeet
I'm not downvoting just yet, in case *I'm* the one to have missed the point - but I believe the problem isn't the cleanup side of things (which would be better off in a teardown if you really want it to happen at the end of the method) but that the test isn't actually doing what it's meant to.
Jon Skeet
I agree Justin probably wants to rewrite this as 3 separate tests. He hasn't shown the initialization code, so it's hard to know if what I'm suggesting is ideal. For my system, re-initializing the database before every test isn't practical, so I do it like this.
Robert Lewis
But then you're not testing OrderDetail or Schedule. You could delete lines 2 and 3 from the try block and still get the same result - not what's wanted, IMO!
Jon Skeet
I'm trying to avoid 3 separate tests for this reason. Let's say I have 10 classes inheriting the base and I want good code coverage. Now I have to write 10 tests for every method in the base. Add a new method to the base? Write 10 more tests. This is why I made a generic class to inherit the base.
Justin Holbrook
Second thought, if my catch block asserts true, the only way to fail would be for all 3 calls to the helper function to not throw an exception. As long as one of the calls throws an exception the test will pass. I need to move the try catch block into the helper method.
Justin Holbrook