Testing DAO Delete User

Question

I have several Dao classes, including a UserDao, below. The DAOs have many methods, but I'm focussing on deleteUser:

@Override
    public boolean deleteUser(Connection connection,String login) throws MySQLEXContainer.MySQLDBExecutionException, SQLException {
        int rowNum = 0;
        Connection con;
        PreparedStatement statement = null;
        try {
            String query = QueriesUtil.getQuery("deleteUser");
            con = connection;
            statement = con.prepareStatement(query);
            statement.setString(1, login);
            rowNum = statement.executeUpdate();
        } catch (SQLException e) {
            LOGGER.error(e);
            throw new MySQLEXContainer.MySQLDBExecutionException("Bad execution",e);
        }finally {
            ConnectionUtil.oneMethodToCloseThemAll(null ,statement,null);
        }
        return rowNum > 0;
    }

Test class for that Dao:

class MySQLUserDaoTest {
    private Connection connection;
    private PreparedStatement preparedStatement;
    private UserDao userDao;
    private ResultSet resultSet;
    @BeforeEach
    void init() throws SQLException {
        userDao = new MySQLUserDao();
        preparedStatement = mock(PreparedStatement.class);
        connection = mock(Connection.class);
        resultSet = mock(ResultSet.class);
        when(connection.prepareStatement(anyString())).thenReturn(preparedStatement);
    }
    
    @Test
    void deleteUser() throws SQLException, MySQLEXContainer.MySQLDBExecutionException {
        String login = "Login";
        when(connection.prepareStatement(anyString()).executeUpdate()).thenReturn(1);
        boolean result = userDao.deleteUser(connection,login);
        assertTrue(result);
    }
}

Am I using Mockito effectively or could my tests be improved?

Answer 1

What are you testing?

Consider whether or not mocking is really the way you want to go with testing your DAO. Often an integration test / in-memory database test can be simpler / more aligned with the way the DAO changes.

If you do want to go down the mocking approach, then you need to be clear about what it is you're trying to achieve with the mocks. As it stands, your test is essentially, "When I call deleteUser, deleteUsershould returntrue". There's some when` mocking setup, to help that happen.

Things you aren't testing (which you may or may not care about):

• That a statement is actually prepared
• The statement that is prepared (it could be "drop table users")
• The parameters that are passed to the statement
• That a prepared statement is actually executed
• What happens if the execution returns values other than 1 (2 and 0 spring to mind)
• The interactions with QueriesUtil / ConnectionsUtil
• What happens if the statement throws an exception

Other general feedback

• deleteUser is a poor name for a test, it gives me no hint as to what I should expect the test to do, other than it has something to do with deleting users.

• You create a mock(ResultSet.class), however it's never used.

• Your when statement in the test is cluttered which makes it more difficult to read, you could just use the field directly:

  when(preparedStatement.executeUpdate()).thenReturn(1);
  

• You don't need to declare function level variables that are just copies of function parameters. This just aliases the variable connection to something less specific, and adds noise to the function:

  con = connection;
  

• deleteUser takes a login parameter, is this the userId?

• You've declared deleteUser as throwing SQLException. It can't unless oneMethodToCloseThemAll throws it (because otherwise you catch and translate it). If oneMethodToCloseThemAll does throw it, do you want to catch and translate that as well?

• Consider if you want to verify any of your mock calls.

• oneMethodToCloseThemAll looks like it's designed to close multiple things, if they're not null. Consider using try with resources instead.