views:

291

answers:

8

i have this method in my data access class and i have a unit test to make sure it does work properly, but my test doesn't test the exception so my question is should i create a new test to force the exception to be raised or should i just trust that the catch block will work just fine in case of an exception ? is it worth the effort ?

here is an example :

public void UpdateProject(Project project)
    {
        using (var transaction = _session.BeginTransaction())
        {
            try
            {
                _session.Update(project);
                _session.Flush();
                transaction.Commit();
            }
            catch (HibernateException)
            {
                transaction.Rollback();
                throw;
            }
        }
    }
+11  A: 

Ideally, you should try to test every line of code that you can... this would mean forcing an exception to occur to test the exception handler, and make sure this works properly.

In practice, there will always be some lines of code not covered - however, I would force exceptions via mocking in order to test any exception handler that you feel is critical, especially, or that will possibly happen in production.

Reed Copsey
It is always fun to debug a production issue where the crash is in the catch block
Mark
If it's just logging and rethrowing, I wouldn't bother. Probably. As long as I was testing logging elsewhere. If it's doing anything important, I would.
serialhobbyist
Yeah - although rolling back a transaction can be (not always) a pretty heavy-weight process, which I believe should be tested. In this case, I'd most likely test it.
Reed Copsey
@Reed - But in this case you would be mocking the session and the transaction. So you would only be checking the the transaction is rolled back, not the code for the rollback. Plus as the rollback is part of an external API. Is it really worth unit testing something that simply hands over to a (hopefully well tested) 3rd party API?
mlk
@mlk: I'm not suggesting testing all of hibernate, but I AM suggesting testing that your transaction does get rolled back properly, and that this can happen without throwing a new exception, and that your throw statement is being handled properly, and you're getting a proper exception bubbling back up. There's enough going on in that catch, that I'd personally make sure it's doing what I expect. I agree, given a mature 3rd party lib, that it doesn't need a huge amount of testing, but I'd still test it (that's my personal risk assessment, though...)
Reed Copsey
+3  A: 

You could get your session to mock throwing an exception but there is little need to test that a catch works.

It's not wrong to write a test to check a transaction is rolled back if an update throws an exception but at a gut level i think this is taking it too far.

Maybe a more elaborate case would warrant it.

On an extra note, would you not rollback upon any type of exception?

dove
+1  A: 

From the exception name (HibernateException) I would think it's not an exceptional case. So it can happen during normal operation. If that's the case I would make a test that causes the exception to make sure it gets handled properly.

grigy
+8  A: 

In Linux kernel, 80% of bugs in drivers are in error-handling code.

Exceptional handling is the source of many errors and you surely should test all the exception paths.

Of course you can't test every line of code. But the statistic says that programmers pay less attention to exception handling, so you must test them thoroughly.

Pavel Shved
+3  A: 

I hope the transaction will be Rollback if Displose() is called before Commit() therefore do you need the catch at all?

As to other cases, if your catch does more then just log the problem, I think it should be unit tested. However don't let the fact you can not do profit unit testing stop you from doing some unit testing.

Ian Ringrose
+1  A: 

I would certainly test each specific exception that is caught and handled. At least you have some execution path through your handling code which should give you some comfort that nothing else weird will happen when/if you ever encounter such an exception during runtime.

Remember, you only need to test the code you want to work.

MikeJ
+1  A: 

I sure hope you would separately test transaction.Rollback() with the tests that exhaust all possible situations you can think of. However, provided that testing was exhaustive, I would probably not bother raising an exception since there's nothing else in the handler.

ilya n.
+3  A: 

Here are my rules for ideal testing of failure behavior:

  • If you are using dependencies that might throw exceptions, then you should include unit tests that make your mocks of them throw exceptions.
  • If under certain conditions your code should throw exception, then you should include unit tests that sets up such conditions.

The first lets you know what your code behaves like under external failure, and often leads to reconsiderations of what to do. In all situations it is good to see what actually happens. The second just makes sure that you hold what you promise, as far as the errors your code should detect and consider exceptional. It is no different than testing other features in your code.

Before considering the unit-test suite complete, one should have a peek at code-coverage of the code under test. For non-trivial code, there is almost always one way my code branch that my unit tests do not cover, or worse, has behaviour I did not intend. Surprisingly often, the solution is to remove that code rather than adding more tests. Just cruft left behind when winging it earlier.

I'm not necessarily saying that one should reach 100% coverage, but one should do code coverage driven unit testing because it is a more visual way to understand and expose the code behaviours that exists. Just looking at it can give you new ideas for how to refactor code to achieve Single Responsibility Principle and Separation of Concerns.

Christian