Thursday, August 31, 2006

Unit Tests Specify Post-Conditions, Not Code Paths!

I realized something about the unit tests I had been writing today: I couldn't change even a line of the code they were testing without changing the tests themselves as well. Now, I understand that unit tests always carry some maintenance overhead, and will need to be udpated from time to time as a system's design evolves. But unit tests are also supposed to enable refactoring by ensuring that there are no unintended consequences to a code modification. If I need to change my test every time I change the code it's testing, then I must be doing something wrong. But I had to think for a minute about what unit testing sin I was committing.

The code being developed did lots of JDBC stuff, so I was using EasyMock to mock the JDBC API, preventing a dependency on a physical database.

Here is a portion from an offending unit test:
  @Override
  protected void setUp() throws Exception {
    ds = createStrictMock(DataSource.class);
    conn = createStrictMock(Connection.class);
    ps = createStrictMock(PreparedStatement.class);
    md = createStrictMock(DatabaseMetaData.class);
    rs = createStrictMock(ResultSet.class);
    xaDsFactory = createMock(XaDataSourceFactory.class);
    defFactory = createMock(DefinitionFactory.class);
    typeMap = createMock(TypeMap.class);
    expect(defFactory.getTypeMap()).andStubReturn(typeMap);
    expect(typeMap.integer()).andStubReturn("INTEGER");
    db = new DatabaseHelperImpl(ds, xaDsFactory, "sqlDialect", defFactory);
    replay(defFactory, typeMap);
  }
  
  public void testExecuteStatement() throws Exception {
    expect(ds.getConnection()).andReturn(conn);
    expect(conn.prepareStatement("SQL Statement")).andReturn(ps);
    expect(ps.execute()).andReturn(false);
    conn.close();
    replay(ds, conn, ps);
    
    db.executeSql("SQL Statement");
    verify(ds, conn, ps);
  }
  
  public void testExecuteSqlThatThrowsException() throws Exception {
    expect(ds.getConnection()).andReturn(conn);
    expect(conn.prepareStatement("Failing SQL Statement")).andReturn(ps);
    SQLException exToThrow = new SQLException();
    expect(ps.execute()).andThrow(exToThrow);
    conn.close();
    replay(ds, conn, ps);
    
    try {
      db.executeSql("Failing SQL Statement");
      fail("Should throw exception");
    catch (SQLException ex) {
      assertEquals("Wrong exception", exToThrow, ex);
    }
    verify(ds, conn, ps);
  }


And here is the Code Under Test:
  public void executeSql(String sqlthrows SQLException {
    Connection conn = null;
    try {
      conn = dataSource.getConnection();
      conn.prepareStatement(sql).execute();
    finally {
      if (conn != nullconn.close();
    }
  }


Oy. Thats' pretty ugly-looking to me. Notice the large setUp() method. Notice that, worse than being large, the method does not actually finish setting up, since each test requires a very particular series of EasyMock calls. To add new tests, you would have to pay very careful attention to setUp() unless you were already familiar with the code.

Taking a minute to stare at these tests, the problem becomes obvious: my tests are essentially a line-by-line walk-through of the desired execution path through the code being tested. This is conceptually only a couple steps away from executing two copies of the same code and verifying that they did the same thing. Not only does this couple my tests to the tested code as tightly as you can imagine, it makes for a pretty good chance that I'll make the same mistaken assumptions in my test that I would make in my code, defeating the purpose of unit testing altogether.

So let me re-consider what a unit test should be. A unit test and the code it tests are just two ways of saying the same thing; two ways of specifying the same desired behavior. Production code specifies a unit of behavior in terms of an arrangement of more finely-grained behaviors. A unit test specifies a unit of behavior by describing the desired post-conditions (outputs and side effects) for a given set of pre-conditions. The tests above are bad tests because they are specifying a unit of behavior in terms of more finely-grained behaviors, just like the code. I am essentially missing the forest (the desired results) for the trees (the steps which will obtain the desired result).

Coming back to the specific problem at hand, I think the reason I was suckered into my predicament has to do with how heavily the code under test relies on the rather large JDBC API to bring about its results. Almost every non-conditional statement in the code under test interacts with the API, and it could be said that the API holds quite a bit of 'state' (the state of the entire database being accessed) which affects its outputs. For these reasons, mocking it in the traditional fashion just doesn't work well.

On my first attempt at addressing the problem, I tried writing the tests against an actual database and verifying post-conditions with JDBC. I suppose this could be made to work, but JDBC does not lend itself well to clearly readable specifications of that kind. And on top of that, the tests were pretty slow.

After pondering a bit, I got to thinking how it would be nice if I had some magical object which could tell me about the things which had been done through the JDBC API after-the-fact, and also allow me to specify how a particular set of JDBC instances will respond, at a high level. Over the course of a few hours I started fleshing out just such an object, relying upon proxies. It took some experimentation (I've never written dynamic proxying code before), but here's the interface I've established for my magical 'DbFixture' object (the code turned out to be rather longer than I can post in here):

interface DbFixture {
  public DataSource getDataSource();
  public int numOpenConnections();
  public boolean wasExecuted(String sql);
  public boolean wasExecuted(String sql, Object[] args, int[] types);
  public void throwOnNextStatementExecution(Throwable ex);
  public int getTxNumForExecutedStatement(String sql);
}


The behavior of getTxNumForExecutedStatement() requires elaboration: it returns -1 if the statement has not been executed, 0 if it was not executed in a transaction, and a number greater than or equal to one if it was executed in a transaction. Transactions are numbered according to the order in which they are ended.

Armed with DbFixture, I rewrote the above tests:

  protected void setUp() throws Exception {
    fixture = new DbFixture();
    xaFactory = createMock(XaDataSourceFactory.class);
    defFactory = createMock(DefinitionFactory.class);
    db = new DatabaseHelperImpl(fixture.getDataSource(), xaFactory,"SQL DIALECT", defFactory);    
  }
  
  public void testExecutingSql() throws Exception {
    db.executeSql("SOME SQL");
    assertTrue(fixture.wasExecuted("SOME SQL"));
  }
  
  public void testThatConnectionIsClosedOnException() throws Exception {
    fixture.throwOnNextStatementExecution(new SQLException());
    try {
      db.executeSql("THROWS EXCEPTION");
      fail("Should have thrown exception");
    catch (SQLException ex) {}
    assertEquals(0, fixture.numOpenConnections());
  }


Much better. Moral of the story: Unit tests specify post-conditions, not code paths!

2 comments:

Sven Heyll said...

Hi Matt,
I am also exploring the wonderfull world of TDD. I have written an answer to your post, wich can be read here:

http://coffeedrivenjava.blogspot.com/2006/08/unit-tests-specify-post-conditions-not.html

Matt McGill said...

Thanks Sven!

(By the way, did you mean to use this URL?
http://sheyll.blogspot.com/2006/09/purpose-of-mock.html)