An Obsession with Everything Else

http://www.derrickschneider.com/atom.xml

Wednesday, December 31, 2008

Unit Testing The UI

The conventional wisdom on unit tests is that they stop at the UI. There are, after all, QA-centric tools that automate the tasks of pressing buttons, choosing menu items, and putting text into forms.

But a lot of what I do at work is writing code that generates web pages. It's been relatively easy to introduce unit tests into utility classes or the service layer, but I wanted to give myself the same stability in the UI universe. Since my team is good about separating models, views, and controllers, I figured out an easy way to bring unit tests up against the UI: Execute methods in the controller, and then inspect the resulting model.

We use the Spring framework, which makes this concept pretty easy. It even includes mock objects for setting up HTTP requests and responses. The generic Spring flow enforces good separation: Your controller's handle method gets called and is expected to return a ModelAndView object. Your unit test can call the same method with appropriately configured mock request objects and then peer into the model object to make sure that values are set properly.

Since our views are usually JSPs, which require a running web container, I don't think there's a good way to write unit tests for that layer. But since the view relies on the model being correct, and the unit tests can look at the model, I think it's a pretty good stopping point. I don't want to rewrite those automated button-pushing tools, after all.

Trying to incorporate this idea into my Mac/iPhone programming, I realized I could go a little bit further than I could in the web container world. I'm working on an iPhone app to enable crazy Derrick meal planning, and one of the controllers is responsible for showing me things I have to do on any one day. I started with that as an experiment. To enable my "UI-ish" unit tests, I have split Apple's default template for this controller into a definite Model object and a definite "ViewLogic" object. The ViewLogic object handles code for formatting the date, determining if the table is present, how many sections it has, and what the title of each section is. All of these vary depending on the state of the model, because there are different categories of things to worry about for any given day, and you could have 0-3 of those categories. My test invokes methods on my controller that update the model object. Then I inspect that model and execute various methods on the ViewLogic object to make sure they're returning correct values based on the model.

Now I can refactor the methods in ViewLogic, some of which have a relatively high cyclomatic complexity, with a high confidence that I won't break the code that relies on it or introduce new bugs.

There are a couple of consequences to this, though. One is a bit of class bloat. I can re-use the model object for at least one other view, but the ViewLogic object is tailored to that one view. The other consequence is that the ViewLogic has become an adapter, converting the methods that the framework expects to find (which have context about views and such that don't exist in the unit test world) into methods I define. Rather than doing the normal thing, then:

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
// do calculations about number of rows
}


I do this:

- (NSInteger)tableView:(UITableView *)tableView numberOfRowsInSection:(NSInteger)section {
return [self rowsInSection: section];
}


Not necessarily bad, but it does make those methods a little anemic. I could pass nil as the table view, but I could see my code at some point needing to execute a method on the table view. So better to isolate "number of rows" logic in a method where I can call it even if I don't have a runtime UITableView object.

Sunday, December 28, 2008

Annotation Processors

Java 1.5 introduced the concept of annotations, markup that you can attach to classes, fields, and methods. We didn't use them a lot at my last job, but in my last round of interviews the subject came up, and I spent some time researching them. At my new job, we — or, rather, the frameworks we use — use them heavily.

I've come to really like them. Like any technology, they can become a hammer that makes everything look like a nail, but they offer a powerful layer on top of code. In particular, they can be used to add cross-cutting functionality that shouldn't be shoehorned into every object. To take one example, you could set up an "Audited" annotation that would fire every time a method is called and would write out the invoking information to a log file. The audited method wouldn't know anything about it and wouldn't need to support the auditing infrastructure: The framework around the method would see the annotation and do the work.

Because they are treated as code, they serve as more-correct documentation. If you the programmer see an @Audited annotation, you know that the given method gets audited. Contrast that with a normal Javadoc comment, which might reflect the method's behavior when it was first written (not audited) but not the current behavior (audited).

Working with annotations in running code is fairly easy. Java's reflection layer, which lets you inspect and manipulate runtime objects, has simple methods that tell you if a given structural element has a given annotation.

But I was intrigued by the concept of annotation processors, compile-time parsers that work off of the annotations in the source. Sun's primary use case for an annotation processor is generating additional files based on an annotation (classes to support XML marshalling and unmarshalling of a given object, for instance). I wondered if you could write an annotation processor that would inspect the source code looking for issues. The earlier you catch problems, the cheaper they are to fix: If I could use annotations to do extra checks on certain code, I could make them compile-time issues that would thus never get released.

I had a particular scenario in mind. At my work, we have some JavaScript code that invokes some of our system's Java objects through indirection, JSON, and reflection. Java supports method overloading (two methods with the same name but different arguments) but JavaScript does not. I inadvertently discovered how this could be a problem when I added an overloaded method to a Java object used by our JavaScript, and it broke our website (in development) for a couple of hours as we tracked down the problem. The JavaScript layer was invoking the new method, not the old method. Now I know this and avoid method overloading in the relevant classes. But some new programmer some day won't know and may make the same mistake. Or I might forget. Every time you have a process that people need to remember, you guarantee that one day someone will forget.

Unfortunately, documentation and examples for this system are sparse. So in the interest of helping others who have similar goals, I've attached the source code below. (There is a Visitor pattern implementation as well, but that is even more poorly documented. I need to revisit my code and figure that out at some point.) The code could use some touch-up, but it gets the idea across. Note that the code uses the Java 1.6 APIs, not the markedly different and unsupported Java 1.5 APIs.

My custom annotation is called RequireUniqueMethodNames and is defined as a class-level annotation. If present, my annotation processor (which you fire by adding a -processor argument to javac) will inspect the class to ensure that there are no overloaded methods. It is smart enough — and the annotation processing system is powerful enough — to distinguish overridden methods (allowed) from overloaded methods (not allowed).

I also allow you to exempt certain method names and not check private methods. These were to support the reality of the legacy code, in which there are overloaded methods that aren't exposed to JavaScript and private methods (which wouldn't be called by anyone) with duplicate names. A refactoring task in my queue will make these attributes irrelevant, but they're there for the moment.

First the annotation declaration:

@Documented
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.CLASS) // required with @Documented
@Inherited
public @interface RequireUniqueMethodNames {
/** Whether or not to enforce duplicate names on private methods */
boolean enforceOnPrivates() default false;

String[] exemptedMethodNames() default {};
}


And now the processor code:

public class UniqueMethodNamesProcessor extends AbstractProcessor {

// stateful!
private boolean enforcePrivates = false;

//stateful!
private List<String> exemptedMethodNames;

public boolean process(Set<? extends TypeElement> annotations,
RoundEnvironment curEnv) {

// each element in the annotations set is a single Annotation
// (and since we only support one, it's always RequireUniqueMethodNames)
for (TypeElement te : annotations) {

// just in case we're passed a stray
if (!te.getQualifiedName().toString().equals("com.ea.sp.community.annotation.RequireUniqueMethodNames")) {
notice("Sent a stray annotation: " + te.getQualifiedName().toString());
continue;
}

for (Element element : curEnv.getElementsAnnotatedWith(te)) {
enforcePrivates = false; // reset each time
exemptedMethodNames = new ArrayList<String>();

// double-check to ensure that each is a class
if (!(element instanceof TypeElement)) {
error("Sent the wrong type of element: " +
element.getKind().name());
continue;
}

TypeElement clazz = (TypeElement)element;

// figure out the annotation instance used on this class
// note that class might have many annotations, but we know ours is
// one of them
for(AnnotationMirror am : elemUtils().getAllAnnotationMirrors(clazz)){
// use elemutils... because we declare this annotation as
// inherited, so we want to make sure we catch the subclasses
if (am.getAnnotationType().asElement().getSimpleName()
.toString().equals("RequireUniqueMethodNames")) {

// todo-dfs: is there a better way to construct the
// ExecutableElement to use for direct lookup?
Map<? extends ExecutableElement,? extends AnnotationValue>
methodMap = elemUtils().getElementValuesWithDefaults(am);
for (ExecutableElement curMethod : methodMap.keySet()) {
AnnotationValue value = methodMap.get(curMethod);
if (curMethod.getSimpleName().toString().
equals("enforceOnPrivates")) {
enforcePrivates =
((Boolean)value.getValue()).booleanValue();
}

if (curMethod.getSimpleName().toString().equals("exemptedMethodNames")) {

List<? extends AnnotationValue> exemptValues =
(List<? extends AnnotationValue>)value.getValue();
for (AnnotationValue arrayValue : exemptValues) {
exemptedMethodNames.add(arrayValue.toString());

}
}

}

}
}

List<TypeElement> classChain = calculateClassHierarchy(clazz);

// this will be a list of method objects that we maintain for
// each class in the list. because we want to check uniqueness
// within the universe of one class, not across the entire
// source base
List<ExecutableElement> methods =
calculateAllSuperMethods(classChain.subList(0,(classChain.size() - 1)));

screenDuplicateMethods(methods,clazz);

}
}
return false;
}

/** Collate all the methods for every incoming TypeElement. This does not do
* a duplicate check because non-annotated classes are allowed to have
* duplicate names.
*/
private List<ExecutableElement>
calculateAllSuperMethods(List<TypeElement> superclasses) {

List<ExecutableElement> retVal = new ArrayList<ExecutableElement>();
for (TypeElement curClass : superclasses) {

for (Element classMember : curClass.getEnclosedElements()) {
if (!classMember.getKind().equals(ElementKind.METHOD)) {
continue;
}
retVal.add((ExecutableElement)classMember);
}
}
return retVal;
}


/** Looks for duplicate methods in the aggregate list of method names that
* have been accrued as we go up the superclass chain.
* @param methodNames the list of ExecutableElements from all the superclasses
* @param curClass the current TypeElement
*/
private void screenDuplicateMethods(List<ExecutableElement> methods,
TypeElement curClass) {

for (Element classMember : curClass.getEnclosedElements()) {


if (!classMember.getKind().equals(ElementKind.METHOD)) {
continue;
}

ExecutableElement method = (ExecutableElement)classMember;
if (method.getModifiers().contains(Modifier.PRIVATE) &&
!this.enforcePrivates) {
continue;
}

// oddly, the method names are recorded as "method name" (with the quotes
// from getannotationvalue. so look for that
if (this.exemptedMethodNames.
contains("\"" +method.getSimpleName().toString()+ "\"")) {
continue;
}

if (!methodNameExists(method,methods)) {
methods.add(method);
continue;
}



//method exists, but is it an override? overrides are okay
if (methodIsOverride(method,methods)) {
continue;
}

error(method.getSimpleName().toString() + " is a duplicate (not an override) of a method elsewhere in class " + method.getEnclosingElement().getSimpleName().toString() + " or in a superclass.");
}
}

/** Recursively constructs a List of TypeElements that form the superclass
* chain for the given class.
* @param classDec the starting class to declare
* @return List of superclasses, with Object at 0 and the passed-in class
* at the end.
*/
private List<TypeElement> calculateClassHierarchy(TypeElement startClass) {
TypeElement superclass =
(TypeElement)typeUtils().asElement(startClass.getSuperclass());
if (superclass == null ||
superclass.equals(typeUtils().getNoType(TypeKind.NONE)) ) {
// we're at Object, so make a list and unrecurse
List<TypeElement> retVal = new ArrayList<TypeElement>();
retVal.add(startClass);
return retVal;
} else {
List<TypeElement> retVal = calculateClassHierarchy(superclass);
retVal.add(startClass);
return retVal;
}
}

/** Does the given ExecutableElement have the same name as something in list
*/
private boolean methodNameExists(ExecutableElement method,
List<ExecutableElement> methods) {
for (ExecutableElement existingMethod : methods) {
if (method.getSimpleName().equals(existingMethod.getSimpleName())) {
return true;
}
}
return false;
}

/** Determines if the passed in method is an override of a method in the list
* Overrides are generally okay even if they have the same name.
*
*/
private boolean methodIsOverride(ExecutableElement method,
List<ExecutableElement> methods) {
for (ExecutableElement superMethod : methods) {
if (elemUtils().overrides(method,superMethod,
(TypeElement)method.getEnclosingElement()) ) {
return true;
}
}

return false;
}

private void notice(String message) {
print(Diagnostic.Kind.NOTE,message);
}

private void error(String message) {
print(Diagnostic.Kind.ERROR,message);
}

private void print(Diagnostic.Kind kind,String message) {
processingEnv.getMessager().printMessage(kind,message);
}

private Types typeUtils() {
return processingEnv.getTypeUtils();
}

private Elements elemUtils() {
return processingEnv.getElementUtils();
}

More Unit Testing Revelations

Since I started making unit tests an active part of my development tasks at home and at work, I've noticed a benefit beyond the promise of stability and support for refactoring.

Unit testing has made me a better programmer. More precisely, it has made me practice what I preach about good programming practice.

One well-known best practice in the OO world — and probably in the procedural programming world, too — is to keep chunks of code small, focused, and free of side effects. This is partly for code maintenance purposes: A large method is cumbersome for a new programmer to understand. But this kind of modularity also allows other programmers (or yourself) to more easily subclass a class and add new behavior to suit new needs.

But in the heat of coding a new feature, I don't always take the time to think about the best way to break it apart. I figure I'll fix it in a refactoring pass. Fifteen years as a professional programmer, and you'd think I know that that chance will never come. Bugs need fixing, or the powers-that-be move me on to a new feature.

With unit tests as part of my task, I'm forced to think about testable chunks, and those are usually exactly the level of size and functionality that makes sense from an object-oriented perspective. I find myself thinking about the tests I will write as I code the class, and that in turn forces me to realize that a given method is too big, has too many side effects, or is otherwise not testable. This is especially true because I have made the decision that our team shouldn't write unit tests for retrieving objects from a database, since that is all handled by the well-vetted Hibernate library. When I write a method that retrieves objects from the database and does calculations with them, the "you'll have to test this" part of my mind realizes that I need to move the calculations into their own method (or methods) so that I can test them even without a database connection. In other words, I move specific functionality into its own method, exactly where it belongs.

Over and over again, every time I code with unit tests in mind, I see myself doing more on-the-fly refactoring as I write a feature. I don't wait for the magical refactoring time that never comes: I do it as I go.