Invariant Properties

Should Mocked Method Arguments Make Assumptions?

Bear Giles | March 27, 2016

I’m cleaning up some tests at my new job and came across an interesting difference in opinion. Here are two ways to write a test. Which is better?

Context: the class being tested acts as a translation layer between our internal abstraction layer and a traditional JDBC driver. JPA and other ORM frameworks are not a viable option since we must work with arbitrary databases. We usually don’t want to call verify() since we want to test behavior, not implementation, but in this case the behavior we want to test is whether the appropriate calls are made to the mocked JDBC objects.

(Implementation note: we are using easymock but we can create similar code in jmock and mockito.)

Approach 1

final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);
// more initialization
expect(conn.prepareStatement("insert into foo(a, b, c) values (?, ?, ?)")).andReturn(stmt);
stmt.setString(1, "Larry");
expectLastCall();
stmt.setString(2, "Curly");
expectLastCall();
stmt.setString(3, "Moe");
expectLastCall();
expect(stmt.execute()).andReturn(true);
// more initialization
replay(conn, stmt);
// run code under test
verify(conn, stmt);
}

final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);

// more initialization

expect(conn.prepareStatement("insert into foo(a, b, c) values (?, ?, ?)")).andReturn(stmt);
stmt.setString(1, "Larry");
expectLastCall();
stmt.setString(2, "Curly");
expectLastCall();
stmt.setString(3, "Moe");
expectLastCall();
expect(stmt.execute()).andReturn(true);

// more initialization

replay(conn, stmt);

// run code under test

verify(conn, stmt);
}

Approach 2

final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);
final String expectedSql = "insert into foo(a, b, c) values (?, ?, ?)";
final List<String> expectedValues = ImmutableList.of("Larry", "Curly", "Moe");
final Capture<String> actualSql = newCapture();
final Capture<String> actualValues = newCapture(CaptureType.ALL);
// more initialization
expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
for (int i = 0; i < expectedValues.size(); i++) {
stmt.setString(eq(i + 1), actualValues);
expectLastCall();
}
expect(stmt.execute()).andReturn(true);
// more initialization
replay(conn, stmt);
// run code under test
assertEquals(expectedSql, actualSql);
assertEquals(expectedValues, actualValues);
verify(conn, stmt);
}

final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);

final String expectedSql = "insert into foo(a, b, c) values (?, ?, ?)";
final List<String> expectedValues = ImmutableList.of("Larry", "Curly", "Moe");

final Capture<String> actualSql = newCapture();
final Capture<String> actualValues = newCapture(CaptureType.ALL);

// more initialization

expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
for (int i = 0; i < expectedValues.size(); i++) {
    stmt.setString(eq(i + 1), actualValues);
    expectLastCall();
}
expect(stmt.execute()).andReturn(true);

// more initialization

replay(conn, stmt);

// run code under test

assertEquals(expectedSql, actualSql);
assertEquals(expectedValues, actualValues);

verify(conn, stmt);
}

Both tests will fail if the expected SQL or values are different than expected. The former creates a framework exception (unexpected method call/missing method call), the latter creates a standard JUnit failure giving expected and actual values.

My coworker argues that the first approach is better since it’s more concise. That’s a good point – shorter methods are easier to understand and maintain.

I argue that the second approach is better since the messages are a lot clearer and the code is much more explicit that we’re concerned about the arguments to the mocked methods. The second approach will also be a lot easier to convert to a parameterized test in the future. Furthermore the actual code (not the snippet above) often uses the expectedValues value as an argument to the code under test. This reduces the risk of breaking tests because we updated one value but not the corresponding value.

Given-When-Then

Academic programs rarely spend much time on testing methodologies and many companies still treat testing as grunt work for the junior people instead of something critical to the long-term success of the project. We would never think it’s enough to just sit down and bang out code but we’ll do exactly that with test code.

No, we need to think about what we’re trying to accomplish and the best way to do it. Fortunately some very smart people have been thinking about this for a long time. One approach is Given-When-Then. Some test frameworks are explicitly built around this approach. JUnit is not but it’s easy to follow as a heuristic.

The second approach is easy to convert to Given-When-Then.

Given-When-Then Approach

Capture<String> setupForPreparedStatement(Connection conn, PreparedStatement stmt, Capture<Object> actualValues)
throws SQLException {
final Capture<String> actualSql = newCapture();
expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
for (int i = 0; i < expectedValues.size(); i++) {
Object obj = expectedValues.get(i);
if (obj == null) {
stmt.setNull(eq(i + 1), anyInt());
} else if (obj instanceof String) {
stmt.setString(eq(i + 1), (String) actualValues);
} else if (obj instanceof BigDecimal) {
stmt.setBigDecimal(eq(i + 1), (BigDecimal) actualValues);
} else ... {
}
expectLastCall();
}
expect(stmt.execute()).andReturn(true);
return actualSql;
}
public void test1() throws SQLException {
given:
final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);
final Capture<String> actualValues = newCapture(CaptureType.ALL);
setupForPreparedStatement(conn, stmt, actualValues);
when:
final List<Object> expectedValues = ImmutableList. of("Larry", "Curly", "Moe");
then:
replay(conn, stmt);
// run code under test
verify(conn, stmt);
assertEquals(expectedValues, actualValues);
}

Capture<String> setupForPreparedStatement(Connection conn, PreparedStatement stmt, Capture<Object> actualValues)
        throws SQLException {
    final Capture<String> actualSql = newCapture();

    expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
    for (int i = 0; i < expectedValues.size(); i++) {
        Object obj = expectedValues.get(i);
        if (obj == null) {
            stmt.setNull(eq(i + 1), anyInt());
        } else if (obj instanceof String) {
            stmt.setString(eq(i + 1), (String) actualValues);
        } else if (obj instanceof BigDecimal) {
            stmt.setBigDecimal(eq(i + 1), (BigDecimal) actualValues);
        } else ... {
        }
        expectLastCall();
    }
    expect(stmt.execute()).andReturn(true);

    return actualSql;
}

public void test1() throws SQLException {
given:
    final Connection conn = createMock(Connection.class);
    final PreparedStatement stmt = createMock(PreparedStatement.class);
    final Capture<String> actualValues = newCapture(CaptureType.ALL);

    setupForPreparedStatement(conn, stmt, actualValues);

when:
    final List<Object> expectedValues = ImmutableList. of("Larry", "Curly", "Moe"); 

then:
    replay(conn, stmt);
    // run code under test
    verify(conn, stmt);

    assertEquals(expectedValues, actualValues);
}

(For clarity I’ve removed the test for the expected SQL statement.)

In this code there is absolutely nothing in the setup that makes assumptions about what values will be passed to the mocked objects. It doesn’t even make assumptions about what SQL commands will be passed, just that we want to create a prepared statement, populate it, then execute it.

It’s not hard to take this a step further and decree that all ‘expect’ calls should be done in setup methods and maintained by the developer working on the tested class. The actual test methods will be maintained by other people and follow the “test behavior, not implementation” rule.

If we don’t need to verify() the mocked objects – and remember we usually won’t since we want to test behavior and not implementation – then we can make the test even more concise.

Given-When-Then Approach without verify()

Capture<String> setupForPreparedStatement(Capture<Object> actualValues) throws SQLException {
final Connection conn = createMock(Connection.class);
final PreparedStatement stmt = createMock(PreparedStatement.class);
final Capture<String> actualSql = newCapture();
expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
for (int i = 0; i < expectedValues.size(); i++) {
Object obj = expectedValues.get(i);
if (obj == null) {
stmt.setNull(eq(i + 1), anyInt());
} else if (obj instanceof String) {
stmt.setString(eq(i + 1), (String) actualValues);
} else if (obj instanceof BigDecimal) {
stmt.setBigDecimal(eq(i + 1), (BigDecimal) actualValues);
} else ... {
}
expectLastCall();
}
expect(stmt.execute()).andReturn(true);
replay(conn, stmt);
return actualSql;
}
public void test1() throws SQLException {
given:
final Capture<String> actualValues = newCapture(CaptureType.ALL);
setupForPreparedStatement(actualValues);
when:
final List<Object> expectedValues = ImmutableList. of("Larry", "Curly", "Moe");
then:
// run code under test
assertEquals(expectedValues, actualValues);
}

Capture<String> setupForPreparedStatement(Capture<Object> actualValues) throws SQLException {
    final Connection conn = createMock(Connection.class);
    final PreparedStatement stmt = createMock(PreparedStatement.class);
    final Capture<String> actualSql = newCapture();

    expect(conn.prepareStatement(capture(actualSql))).andReturn(stmt);
    for (int i = 0; i < expectedValues.size(); i++) {
        Object obj = expectedValues.get(i);
        if (obj == null) {
            stmt.setNull(eq(i + 1), anyInt());
        } else if (obj instanceof String) {
            stmt.setString(eq(i + 1), (String) actualValues);
        } else if (obj instanceof BigDecimal) {
            stmt.setBigDecimal(eq(i + 1), (BigDecimal) actualValues);
        } else ... {
        }
        expectLastCall();
    }
    expect(stmt.execute()).andReturn(true);
    replay(conn, stmt);

    return actualSql;
}

public void test1() throws SQLException {
given:
    final Capture<String> actualValues = newCapture(CaptureType.ALL);
    setupForPreparedStatement(actualValues);

when:
    final List<Object> expectedValues = ImmutableList. of("Larry", "Curly", "Moe"); 

then:
    // run code under test

    assertEquals(expectedValues, actualValues);
}