6
\$\begingroup\$

Use Case

I have a service that does processing of records from multiple third-party feeds. The steps are generally identical for each feed, but each feed is populated in a different location on an aggregate feed. I decided to write a generic test that would allow testing of all this processing to keep things DRY.

Test

The test basically tests whether the processor method in our service properly adapts and adds the incoming record to the appropriate aggregate list.

@Mock AggregateFeed dataFeed; // contains lists of POJOs
@Mock Exchange exchange; // camel mock.. not really important

@Mock SportsAdapter sportsAdapter; // used to convert string to POJO
@Mock BusinessAdapter businessAdapter; // used to convert string to POJO

FeedService fixture = new FeedService();

...

private <T1, T2> void shouldProcessRecord(BiConsumer<T1, Exchange> processor,
        T1 incomingRecord, T2 fakeAdaptee, Function<T1, T2> mockAdapter,
        Function<AggregateFeed, List<T2>> list) {
    List<T2> fakeList = Lists.newArrayList();

    when(mockAdapter.apply(incomingRecord)).thenReturn(fakeAdaptee);
    when(list.apply(dataFeed)).thenReturn(fakeList);

    processor.accept(incomingRecord, exchange);

    assertEquals(1, fakeList.size());
    assertSame(fakeAdaptee, fakeList.get(0));
}

@Test
public void shouldProcessSports() {
    String testRecordFeed = constructSportsRecordFeed();
    SportsRecord fakeSportsRecord = new SportsRecord();
    shouldProcessRecord(fixture::addSportsRecord, testRecordFeed, 
        fakeSportsRecord, sportsAdapter::fromFeed, AggregateFeed::getSports);
}

@Test
public void shouldProcessBusiness() { 
    String testRecordFeed = constructBusinessRecordFeed();
    BusinessRecord fakeBusinessRecord = new BusinessRecord();
    shouldProcessRecord(fixture::addBusinessRecord, testRecordFeed, 
        fakeBusinessRecord, businessAdapter::fromFeed, AggregateFeed::getBusiness);
}

// builds up a csv delimited string that represents a sports article
private String constructSportsRecordFeed() {
    return new StringBuilder().append( ... ).toString();
}

// builds up a csv delimited string that represents a business article
private String constructBusinessRecordFeed() {
    return new StringBuilder().append( ... ).toString();
}

Concerns

The main problem I have doing things this way is that the signature just doesn't look natural. If another developer comes to create a test, they'll have to dig through the code to figure out how to use this test. On the other hand, it's much easier to do a copy and paste of the test and change around the variables, but that doesn't feel right either because it's not DRY.

Questions

  • How can this generic test be improved to be easier to use?
  • Since this is a test, should a copy-and-paste methodology be better since it's more direct on what the test does?
  • Any other advice/comments would be greatly appreciated.
\$\endgroup\$
  • 2
    \$\begingroup\$ In my opinion, DRY applies less to unit tests. Readability and clarity is much more important. \$\endgroup\$ – Captain Man May 9 '15 at 16:31
  • \$\begingroup\$ Is testRecordFeed the same as testSportsRecord, i.e. a typo? Where/what is sportsAdapter? Maybe it'll help to also show us the implementation for at least constructSportsRecordFeed(), if not shouldProcessBusiness() too? These seems to be the critical missing pieces... \$\endgroup\$ – h.j.k. May 15 '15 at 2:09
  • \$\begingroup\$ @h.j.k. I've updated the code to reflect the missing items. In my haste, I forgot to include the mocks for the adapters. I didn't add the details for constructSportsRecordFeed because basically it's a very large csv string. Didn't want to clutter the code. \$\endgroup\$ – Pete May 19 '15 at 14:44
3
\$\begingroup\$

I am only going to talk about the sports method, from there you should be able to get the info you need for the others as they are all basically the same.

DRY isn't a big priority

As I said in my earlier comment, DRY (don't repeat yourself) principle applies to production code more than unit test code. I think even in general it's easy to take this too far, readability is important, but that's off-topic. Your code however is tough to grasp.

To me, something like the below is more clear. I like to specify "given/when/then" in comments (although usually I do nothing more than // Given, etc.) but it's not necessary.

@Test
public void shouldProcessSports() {

    List<SportsRecord> fakeList = Lists.newArrayList();

    // Given an incoming feed of the sports variety
    String testRecordFeed = constructSportsRecordFeed();
    SportsRecord fakeSportsRecord = new SportsRecord();

    Function<AggregateFeed, List<SportsRecord>> list = AggregateFeed::getSports;

    when(mockAdapter.apply(testRecordFeed)).thenReturn(fakeSportsRecord);
    when(list.apply(dataFeed)).thenReturn(fakeList); // (*) See note below

    // When the processor accepts the feed and exchange
    processor.accept(testRecordFeed, exchange);

    // Then there should be one result and it should be the sports record
    assertEquals(1, fakeList.size());
    assertSame(fakeSportsRecord, fakeList.get(0));
}

Note: I don't have much experience with the :: operator, if AggregateFeed::getSports.apply(dataFeed) or something similar is valid I would prefer that instead of making the list variable.

DRY is still relevant though!

While writing tests like this takes up more space I think it's more important that the test be clear.

All that said I don't think DRY is completely useless in unit tests. Suppose you were doing more checks on fakeList, like making sure it's not null, contains a specified number of elements, etc. I would put all those assertions in a separate method. Basically the assertions that are tangential to what you are testing. For example, you're not specifically testing the list has one element above, the assertion you care most about is that the only element is fakeSportsRecord. You can't really do something like that so you have to assert that the first element is fakeSportsRecord. Anyways, I tend to put these tangential assertions in methods like this:

private static void verifyListExistsAndHasSingleElement(List<?> list) {
    assertNotNull(list);
    assertEquals(1, list.size());
}

Naming might be a little poor.

The name are mostly fine, but I felt like I needed to add more.

  • fakeListresultList or outputList
  • testRecordFeedincomingRecordFeed or givenRecordFeed
  • fakeSportsRecordexpectedSportsRecord
  • list, this is by far the worst. As shown in the note I don't know if this variable is even necessary. Even just function is better because it's a function, not a list.
\$\endgroup\$
  • \$\begingroup\$ Thank you for your time and detailed explanation. I really appreciate the part about naming as I've been trying to improve my naming of methods & variables lately. \$\endgroup\$ – Pete Jun 8 '15 at 13:41

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.