Unit testing DAO, am I doing it right?

Question

I'm starting out unit testing and reviewing Java web programming. The thing is I don't know if I'm doing things right.

I'm building a mini blog. I'm using EasyMock for the mock objects.

Here's my code:

The test case

The PostDAO

The Post Bean

I'd really appreciate your comments and suggestions on how I could improve my code and become a better programmer. Thanks in advance!

Answer 1

Not related to your unit testing, but I'd consider your exception handling in createPost(..) bad practise. The exception isn't completely swallowed, but returning a Post object as if everything went okay is not a good idea. See Error Hiding

Answer 2

I think it's generally ok (I'd introduce a String constant for your SQL to avoid repeating it, btw).

Of course, the one thing you're not testing with this is the actual interaction with the database (since this is being mocked out). So I would expect a corresponding test (or set of?) that actually interact with the database, and insert/rollback as appropriate. Otherwise you're testing something that (at the moment) is quite trivial.

A note on test organisation. The below tests 2 things (null construction fails, and normal construction succeeds). I would split this into two tests, otherwise if the first section fails, you never test the second section (in more complex scenarios, this makes fault diagnosis more difficult, since you may not have as much evidence as you require)

public void testPostDAO() {
        try {
                new PostDAO(null);
                fail("Expected IllegalArgumentException");
        } catch (IllegalArgumentException ex) {}
        new PostDAO(connectionMock);
}

Some people object to testing new PostDAO(null) due to its triviality. I disagree. One of the reasons you write tests is to ensure behaviour doesn't change unless you expect it to. So the above is good - I'd just split it into two explicit tests.

Answer 3

Some thought from beginnig of your test:

public void testPostDAO() {
                try {
                        new PostDAO(null);
                        fail("Expected IllegalArgumentException");
                } catch (IllegalArgumentException ex) {}
                new PostDAO(connectionMock);
        }

Following changes are not mandatory and someone can said they are wrong, but it's my opinion:

1. Split normal case and error -- that are two different behaviours of your unit under test

2. Rename it to something more clear - to specify what you are doing -- e.g. testPostDAOWithNullArg

3. Your test failes if no exception raised, how about if WeryBadStange exception will be raised? You will not catch it, and will not get to fail line.

Answer 4

• What are you testing here? IMHO the only thing important here is your SQL command as there's no specific logic to test in your DAO.
• Testing init code like "new PostDAO(null)" is useless when it is so simple.
• I would rather hsqldb in memory mode to test against a real database and have less mock code.
• Is there any reason why you could not use JUnit 4 test style using annotation rather than extending TestCase as in JUnit 3 ?

Answer 5

There is not point in your Unit Test at all. What you are testing? What logic do you have? What it proves?

I can see that your tests prove nothing. Miss typed insert will pass your test. Invalid arguments too.

I think you should use integration or functional testing here. Use real database. This will make your tests much lighter, plus this will test your code and what is more important it will prove that your code correct.

Answer 6

This is a follow-up to another question on SO.

Here are some thoughts for a mini-code review. Take them for what they're worth, just don't take offense:

1. You should be using JUnit 4.x idiom. No need to extend TestCase. Use the "@Test" annotations.
2. Mocking the connection for the DAO is ridiculous. Use a real connection, connect to the database, and run real queries. Without that, your DAO test is worthless.
3. You're missing one of the most important considerations when testing DAOs: the data. Your set up, as written, isn't helping. What you really want is to seed a database with test data, run your tests, and then roll the whole thing back so it's as if you were never there. One of the biggest problems with testing databases is making sure your test data is available. Doing things as one transaction is the best way to accomplish it.
4. You're testing the "happy path", but you aren't trying any edge conditions at all. Each one should be a separate test call. I can't see your schema, but if your table prohibits null values for any of the parameters, or enforces a unique constraint, I'd write separate tests for each to demonstrate that fact and to prove that it's working properly. What's the right approach for handling them? Throw an exception? You should think about it.
5. Your error handling is poor. Printing a message to the console isn't helpful. You should at least log it using log4j.
6. Your createPost method seems off-base. You pass two parameters and return a Post. Usually with ORM solutions, like Hibernate, you'll have objects in place already.
7. DAOs should not be creating Connections. They should be passed in by a service layer that knows about units of work and transactions.
8. Speaking of which, you have no commit/rollback logic. Thank god, because it doesn't belong in a DAO, but I'll bet you haven't thought about it.
9. It looks like you mean "id" to be a primary key, but I see nothing to actually pull it out of the database after your INSERT and populate the object. It won't happen on its own.
10. You'd learn a lot by looking at Spring's JDBC support. I'd recommend it. They've done this better than you ever will.