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:
- It was time consuming to identify the root cause;
- My unit tests didn't address that particular scenario;
- 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;
- 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:
- Identify and fix the class of problems observed;
- Improve prevention and identification of similar problems in the future, by expanding the coverage and improving the quality of the unit-tests;
- Introduce a prevention and notification mechanism;
- Improve the user experience by providing user-friendly error messages;
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:
- send request to the 3rd party translation sub-system with the original message to translate, the source language, and the destination language;
- fetch response from the translation sub-system;
- 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
