Better living through (unit) testing and advice

A few weeks ago I (accidentally) found out that a pet-project of mine, the cross-lingual instant messaging, wasn't working anymore. Debugging showed the "3rd party translation sub-system" was always returning an empty translation.

The fix revealed my development and maintenance process was flawed:

1. It was time consuming to identify the root cause;
2. My unit tests didn't address that particular scenario;
3. I wasn't notified when the problem first occurred, and so might have been unaware of the existence of a problem for God knows how long;
4. There was no user friendly message given to the user, who was left with emptiness where a translated message was expected;
   

The course of action required addressing each of the above listed issues:

1. Identify and fix the class of problems observed;
2. Improve prevention and identification of similar problems in the future, by expanding the coverage and improving the quality of the unit-tests;
3. Introduce a prevention and notification mechanism;
4. Improve the user experience by providing user-friendly error messages;
   

1) Identify and fix the class of problems observed
I just had to find out what changed in the 3rd party API and adapt my parser accordingly. Thanks to Spring, this involved a small change on a XML file and didn't require recompiling the code.
Now the next step forward is trying to automate this detection and adaption process. This is still work in progress to merit a post of its own.

2) Improve coverage and quality of unit-tests
Unit-tests that don't address particular scenarios are text-book cases of code smells. Here's the signature for my translation services:

public interface ITranslationEngine {
  /**
    * Translates 'text' from a given 'fromLanguage' into a given 'toLanguage'
    * @param fromLanguage
    * @param toLanguage
    * @param text
    * @return
    * @throws Exception
  */
  public String translate(String fromLanguage, String toLanguage, String text) throws TranslationException;
}

Implementations of the method translate above were responsible for the 3 tasks below:

1. send request to the 3rd party translation sub-system with the original message to translate, the source language, and the destination language;
2. fetch response from the translation sub-system;
3. parse response, extract and return translated string;
   

There weren't any unit-tests for any of these tasks because these multiple tasks were encapsulated under the one single method translate. When a method is long and/or does too much, it is harder to test. The method wasn't fine-grained enough to allow proper unit-test coverage, and there were at least two anti-patterns present in the tests:

• The One, where a unit-test contains one test method which tests an entire set of functionality;
• The Superficial Coverage, where exceptional conditions are missing from the test cases;
  

The fix involved extracting methods to simplify testing. The original translate method was refactored and each of the tasks listed above extracted into a well-named method. I added unit-tests that focused on each extracted method and considered input and output data in valid and exceptional scenarios. Loose coupling and testability go hand in hand.

3) Introduce a notification mechanism
To address this I implemented an Advice, a Spring schema-based AOP, to intercept the translate method, and check its arguments and return String. If the returned string is empty, a notification email is sent. The code snippet is shown below:

...
<aop:config>
   <aop:aspect id="translationEngineInterceptor" ref="translationEngineAdvice">
      <aop:after-returning
          method="notify"
          returning="translatedString"
          pointcut="execution(* translate(String, String, String)) and args(from, to, text)" />
   </aop:aspect>
</aop:config>

<bean id="translationEngineAdvice" class="org.jiim.translation.TranslationEngineAdvice">
    <property name="mailSender" ref="mailSender"/>
    <property name="mailMessage" ref="mailMessage"/>
</bean>

...

public class TranslationEngineAdvice {
  protected final Log logger = LogFactory.getLog(getClass());
  private MailSender mailSender;
  private SimpleMailMessage mailMessage;
  public static final String NEW_LINE = System.getProperty("line.separator");

  ...

  public boolean notifyByEmail(JoinPoint jp, Object translatedString, String from, String to, String text) {
    String s = (String) translatedString;
    // if message string returns empty translation
    if (!"".equals(text) && "".equals(s.trim())) {
      SimpleMailMessage msg = new SimpleMailMessage(this.mailMessage);
      StringBuffer txt = new StringBuffer(msg.getText());
      txt.append(NEW_LINE).append("Date [").append(new Date()).append("]");
      txt.append(NEW_LINE).append("From language [").append(from).append("]");
      txt.append(NEW_LINE).append("To language [").append(to).append("]");
      txt.append(NEW_LINE).append("Original Text [").append(text).append("]").append (NEW_LINE);
      msg.setText(txt.toString());
      try {
        this.mailSender.send(msg);
        return true;
      } catch (MailException me) {
        logger.error("Error sending email notification ", me);
      }
    }
    return false;
  }
}
...

4) Improve the user experience
I made a more considerate application by fetching a localized user friendly message to display if the translated message is empty.

References
Extracting Methods to Simplify Testing
Making Considerate Software
JUnit Anti-Patterns
TDD Anti-Pattern Catalogue
Reusable Advice - Part II: Unobtrusive Notification

CRUD unit-testing checklist

I'm writing a unit-test code generator for CRUD operations to keep me from repeating myself. In preparation for this, I'm compiling a comprehensive set of CRUD unit-tests beyond the eponymous ones.

So, for a given business object Foo I may want to test:

Create

public void TestCreate_NullFoo {
   try {
       PersistenceServices.create(Null);
   } catch(PersistenceServiceException pse){
       return;
   }
   fail("Expected exception not thrown");
}

// possible test, depends on requirements
public void TestCreate_EmptyFoo {
   Foo foo = new Foo(); // no properties set
   try {
       PersistenceServices.create(foo);
   } catch(PersistenceServiceException pse){
       return;
   }
   fail("Expected exception not thrown");
}

// depends on requirements; validation shouldn't happen here
public void TestCreate_InvalidFoo {
   Foo foo = new Foo();
   foo.set...; // populate with known invalid values
   try {
       PersistenceServices.create(foo);
   } catch(PersistenceServiceException pse){
       return;
   }
   fail("Expected exception not thrown");
}

// depends on requirements; validation shouldn't happen here
public void TestCreate_IncompleteFoo {
   Foo foo = new Foo();
   foo.set...; // miss required values
   try {
       PersistenceServices.create(foo);
   } catch(PersistenceServiceException pse){
       return;
   }
   fail("Expected exception not thrown");
}

public void TestCreate_IncrementsCount {
   int beforeCreate = PersistenceServices.countFoos();
   Foo foo = new Foo();
   foo.set...; // populate with required values
   PersistenceServices.create(foo);
   int afterCreate = PersistenceServices.countFoos();
   assertTrue(afterCreate > beforeCreate);
}

public void TestCreate_IsNotIdempotent {
   Foo foo = new Foo();
   foo.set...; // populate with required values
   Foo aCreated = PersistenceServices.create(foo);
   Foo bCreated = PersistenceServices.create(foo);
   assertNotEquals(aCreated, bCreated);
}

public void TestCreate_Foo {
   Foo foo = new Foo();
   foo.set...; // populate with required & validated values
   Foo created = PersistenceServices.create(foo);
   assertEquals(foo, created);
}

Read

public void TestRead_NullFoo {
   Foo read = PersistenceServices.getFoo(Null);
   assertNull(read);
}

// depends on requirements
public void TestRead_EmptyFooId {
   Foo read = PersistenceServices.getFoo("");
   assertNull(read);
}

public void TestRead_DoesNotIncrementCount {
   int beforeRead = PersistenceServices.countFoos();
   PersistenceServices.getFoo(aFoo); // aFoo is known, existing Foo
   int afterRead = PersistenceServices.countFoos();
   assertTrue(beforeRead == afterRead);
}

public void TestRead_DuplicateFoo { // see IsIdempotent below
}

public void TestRead_IsIdempotent {
   Foo aFoo = PersistenceServices.getFoo(aFoo);
   Foo bFoo = PersistenceServices.getFoo(aFoo);
   assertEquals(aRead, bRead);
}

// depends on requirements; could throw friendly exception
public void TestRead_DeletedFoo {
   Foo deleted = PersistenceServices.delete(aFoo);
   Foo read = PersistenceServices.getFoo(deleted);
   assertNull(read);
}

public void TestRead_FooById {
   Foo foo = new Foo();
   foo.set...; // populate with required & validated values
   Foo created = PersistenceServices.create(foo);
   Foo read = PersistenceServices.getById(created.id);
   assertEquals(foo, read);
}

Update

public void TestUpdate_NullFoo {}

public void TestUpdate_EmptyFoo {}

public void TestUpdate_InvalidFoo {}

public void TestUpdate_IncompleteFoo {}

public void TestUpdate_DuplicateFoo {}

public void TestUpdate_UnchangedFoo {}

public void TestUpdate_DoesNotIncrementCount {}

public void TestUpdate_IsIdempotent {}

public void TestUpdate_UndoFoo {}

public void TestUpdate_Foo {}

Delete

public void TestDelete_NullFoo {
   Foo deleted = PersistenceServices.delete(Null);
   assertNull(deleted);
}

public void TestDelete_EmptyFoo { // dependent of business requirements
   Foo foo = new Foo(); // no properties set
   Foo deleted = PersistenceServices.delete(foo);
   assertNull(deleted);
}

public void TestDelete_InvalidFoo {
}

public void TestDelete_IncompleteFoo {
}

public void TestDelete_DuplicateFoo {
}

public void TestDelete_DecrementsCount {
   Foo aFoo = PersistenceServices.getFoo(aFoo);
   int beforeDelete = PersistenceServices.countFoos();
   PersistenceServices.delete(aFoo);
   int afterDelete = PersistenceServices.countFoos();
   assertTrue(beforeDelete > afterDelete);
}

public void TestDelete_IsIdempotent {
   Foo aFoo = PersistenceServices.delete(aFoo);
   Foo bFoo = PersistenceServices.getFoo(aFoo);
   assertNull(bFoo);
   aFoo = PersistenceServices.delete(aFoo);
   bFoo = PersistenceServices.getFoo(aFoo);
   assertNull(bFoo);
}

public void TestDelete_ExistingFoo {
   Foo aFoo = PersistenceServices.delete(aFoo);
   assertNotNull(aFoo);
   Foo bFoo = PersistenceServices.getFoo(aFoo);
   assertNull(bFoo);
}

public void TestDelete_MissingFoo {
   // aFoo does not exist
   Foo deleted = PersistenceServices.delete(aFoo);
   assertNull(deleted); // could expect exception
}

// depends on requirements
public void TestDelete_ReturnsDeleted {
   Foo aFoo = PersistenceServices.getById(aFoo.id);
   Foo deleted = PersistenceServices.delete(aFoo);
   assertEquals(aFoo, deleted);
}

(Vaguely) Related Posts
Testing For Developers
Automated Unit Testing
CRUD base classes for unit testing
Unitils Guidelines