3
\$\begingroup\$

I have a class which inserts a list of tweets to a database table and also inserts the tweet's key with the associated filter's key to a many-to-many table. I am testing the sole public method of this class:

class TweetByFilterDatabaseRepository {

    private final DatabaseFacade database;

    private final Mapper<TweetEntity, ContentValuesWrapper> toTweetContentValuesMapper;

    private final TwoToOneMapper<SearchFilterEntity, TweetEntity, ContentValuesWrapper>
            toTweetAndSearchFilterContentValuesMapper;

    private final Mapper<Cursor, TweetEntity> toTweetMapper;

    TweetByFilterDatabaseRepository(DatabaseFacade database,
                                    Mapper<TweetEntity, ContentValuesWrapper> toTweetContentValuesMapper,
                                    TwoToOneMapper<SearchFilterEntity, TweetEntity, ContentValuesWrapper>
                                            toTweetAndSearchFilterContentValuesMapper,
                                    Mapper<Cursor, TweetEntity> toTweetMapper) {
        this.database = database;
        this.toTweetContentValuesMapper = toTweetContentValuesMapper;
        this.toTweetAndSearchFilterContentValuesMapper = toTweetAndSearchFilterContentValuesMapper;
        this.toTweetMapper = toTweetMapper;
    }

    public void addAll(SearchFilterEntity filter, List<TweetEntity> tweets) {
        database.beginTransaction();

        try {
            for (TweetEntity tweet : tweets) {
                try {
                    saveTweet(tweet);
                    saveTweetSearchFilterForeignKeys(filter, tweet);
                } catch (MappingException e) {
                    //no-op
                }
            }
            database.setTransactionSuccessful();
        } finally {
            database.endTransaction();
        }
    }

    private void saveTweet(TweetEntity tweet) throws MappingException {
        final ContentValuesWrapper tweetContentValues = toTweetContentValuesMapper.mapOrThrow(tweet);
        database.insertOrUpdate(TweetSchema.TABLE_NAME, tweetContentValues, TweetSchema.COLUMN_ID);
    }

    private void saveTweetSearchFilterForeignKeys(SearchFilterEntity filter,
                                                  TweetEntity tweet) throws MappingException {
        final ContentValuesWrapper tweetAndSearchFilterContentValues
                = toTweetAndSearchFilterContentValuesMapper.mapOrThrow(filter, tweet);
        database.insertOrIgnore(TweetSearchFilterSchema.TABLE_NAME, tweetAndSearchFilterContentValues);
    }
}

I want to test the two functionalities, that each Tweet is inserted to the database and that each pair is inserted to the database. I am testing the first functionality like so:

@RunWith(MockitoJUnitRunner.class)
public class TweetByFilterDatabaseRepositoryTest {

    @Mock private DatabaseFacade database;
    @Mock private Mapper<TweetEntity, ContentValuesWrapper> toTweetContentValuesMapper;
    @Mock private TwoToOneMapper<SearchFilterEntity, TweetEntity, ContentValuesWrapper>
            toTweetAndSearchFilterContentValuesMapper;
    @Mock private Mapper<Cursor, TweetEntity> toTweetMapper;
    private TweetByFilterDatabaseRepository tweetByFilterDatabaseRepository;

    @Before
    public void beforeEach() {
        tweetByFilterDatabaseRepository = new TweetByFilterDatabaseRepository(database, toTweetContentValuesMapper, toTweetAndSearchFilterContentValuesMapper, toTweetMapper);
    }

    @Test
    public void addingAllItems_withoutErrors_addsTweetsToDatabase() throws MappingException {
        SearchFilterEntity filter = mock(SearchFilterEntity.class);
        List<TweetEntity> tweets = mockedListOf(TweetEntity.class);
        List<ContentValuesWrapper> contentValues = mockedListOf(ContentValuesWrapper.class);
        for (int i = 0; i < tweets.size(); i++) {
            TweetEntity tweet = tweets.get(i);
            when(toTweetContentValuesMapper.mapOrThrow(tweet)).thenReturn(contentValues.get(i));
        }

        tweetByFilterDatabaseRepository.addAll(filter, tweets);

        for (int i = 0; i < tweets.size(); i++) {
            TweetEntity tweet = tweets.get(i);
            ContentValuesWrapper contentValuesWrapper = contentValues.get(i);
            verify(toTweetContentValuesMapper).mapOrThrow(tweet);
            verify(database).insertOrUpdate(TweetSchema.TABLE_NAME, contentValuesWrapper, TweetSchema.COLUMN_ID);
        }
    }
}

Is there a better way to test this without the for loops or is maybe my design flawed? I am primarily interested in removing the loops from the unit test but I am also interested in improving the design of the main class if it is needed.

\$\endgroup\$
  • \$\begingroup\$ What is mockedListOf? This isn't a Mockito method. \$\endgroup\$ – Tunaki May 13 '16 at 15:01
  • \$\begingroup\$ @Tunaki it's a helper method that I made. It basically creates a list of mocks of the class specified. \$\endgroup\$ – Aki K May 13 '16 at 15:20
  • \$\begingroup\$ Btw. unless I'm missing something there's also @InjectMocks instead of the manual constructor call. \$\endgroup\$ – ferada May 14 '16 at 7:40
  • \$\begingroup\$ @ferada that's a great tip thanks for letting me know. \$\endgroup\$ – Aki K May 14 '16 at 13:26
2
\$\begingroup\$

Disclaimer: I'm not a huge fan of mocks.

Some issues I see with the approach:

  • The fixture setup is quite complicated, involving a lot of mocks. It's a lot to read.
  • Using loops in test methods is a test smell.
  • Verifying that .mapOrThrow gets called on each tweet, and verifying that insertOrUpdate gets called with specific parameters on database call for concern. The test knows too much about intimate details of the implementation. As such, the tests are too tightly coupled to the implementation, and very likely fragile tests (another test smell). The slightest change in implementation details will likely break tests, and become a maintenance nightmare.

I suggest to take a different approach: use a fake database. With an in-memory implementation of DatabaseFacade, the test class could become a lot simpler, easier to read, and eliminate the current test smells.

//@RunWith(MockitoJUnitRunner.class) -> not needed anymore -> to delete
public class TweetByFilterDatabaseRepositoryTest {

    @Test
    public void addingAllItems_withoutErrors_addsTweetsToDatabase() {
        DatabaseFacade database = newFakeDatabase();
        SearchFilterEntity filter = mock(SearchFilterEntity.class);
        List<TweetEntity> tweets = mockedListOf(TweetEntity.class);

        tweetByFilterDatabaseRepository.addAll(filter, tweets);

        assertEquals(tweets, database.getTweets());
    }
}

I mentioned at the top that I don't like mocks. But notice that I didn't eliminate all the mocks. I only eliminated the ones that didn't help reducing complexity. I kept the ones that are useful, and I have no problems with those.

It should be straightforward to add further tests for the cases of some tweets with mapping errors, all tweets with mapping errors, filtering logic, and so on.

\$\endgroup\$
  • \$\begingroup\$ Absolutely, while I'm a fan of Mockito that's exactly what will happen later on. The thing I'd like to add is that using a different database engine for the tests gives a somewhat false sense of safety if there's no further integration test with the real one (i.e. H2 vs. PostgreSQL is quite a difference in behaviour). \$\endgroup\$ – ferada May 14 '16 at 7:37
  • \$\begingroup\$ I want to stop using mockito (and mocks in general) that much but I feel like by going your way I will be duplicating an SQLite database. I would have to implement a way to insert data to the in-memory database by table (therefore implementing tables of some kind) and I would also have to implement a way to query those tables (there is no getTweets, what exists is database.query(tableName, args, etc.)) which would possibly mean even more work. That gets even more complicated if I have to do a getTweetsByFilter which requires joins. \$\endgroup\$ – Aki K May 14 '16 at 13:31
  • \$\begingroup\$ Not really. Just like you the table name doesn't matter when you spy on method calls, it doesn't have to matter in your fake database. The fake database could simply disregard the table name parameter, and always insert to a simple list, if that's enough for the test case to work. You only implement just enough for a test case to work. This might take some getting used to, but overall the code will be shorter and easier to understand, and less coupled to the underlying implementation. \$\endgroup\$ – janos May 14 '16 at 13:55
0
\$\begingroup\$

One improvement would be to extract mappers part to separate class. Then, you could add addAll(List<ContentValuesWrapper> contents) method to your repository and verify invocations of that single method. That would probably simplify testing a little bit.

\$\endgroup\$
  • \$\begingroup\$ The problem is that I am inserting in two different tables in the database where one is the TweetSchema.TABLE_NAME and the other is the TweetSearchFilterSchema.TABLE_NAME and I am also inserting with two different strategies (one is insertOrUpdate and the other insertOrIgnore) \$\endgroup\$ – Aki K May 13 '16 at 15:25
  • \$\begingroup\$ Right. Actually, you probably test mappers in differents test suits. Maybe it would be enough to simply verify: verify(database, times(tweet.size())).insertOrUpdate(any(ContentValuesWrapper.class)); here \$\endgroup\$ – Jakub Bibro May 13 '16 at 15:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.