100% code coverage, Hibernate Validator and Design by Contract

Code coverage in unit testing was one of the first things I learned in my software engineering career. The company I worked at taught me that you should have 100% coverage as a goal, but achieving it does not mean there are no bugs in the system. At that time, I worked at a company whose big thing was that they delivered very reliable billing software to telecomms operators. We used to invest as much time writing unit tests as we did writing the code. If you included unit tests, integration tests, system tests and acceptance tests, more time was spent testing than designing and implementing the code which ran in production. It was impressive and I have never worked with or for a company with that kind of model since, although I am sure there are many companies which operate like that.

The other day I was thinking back to those days and reading up about unit testing to refresh myself in preparation for a unit testing course I'm attending soon (don't want the instructor to know more than me!) and I started to wonder about what kind of code could be fully covered during unit testing, but which could still contain a bug. While I learned that 100% coverage should be the goal, in over 10 years I have never worked on a project which achieved that. So I have never surprised to find a bug in code which I thought was "fully" tested.

It's fun write whacky code - you don't normally try and write bugs :-) I started with the specification:

    /**
     * @return String "a" or "b" depending upon the values
     * passed to the init method. If variable "a" is true,
     * then string "a" is returned. If variable "b" is true,
     * then "b" is returned. If neither is true, then "" is
     * returned. Variable "b" is important than "a", so if
     * both are true then return "b".
     */

Now, the tricky part was to write code which contained a bug yet was easy to cover with incomplete tests. I came up with the following. The specification above is for the "getResult()" method.

public class SomeClass {

    private boolean a;
    private boolean b;

    public SomeClass init(boolean a, boolean b) {
        this.a = a;
        this.b = b;
        return this;
    }

    public String getResult() {
        String s = "";
        if(a) {
            s = "a";
        }
        if(b){
            s += "b"; // <-- oops!
        }
        return s;
    }
}

The "bug" is in the method "getResult" and the problem is that instead of just an assignment, the "plus equals" operator has been used. An "else" would probably make the code a little safer too. But I think code like this is quite typical of the buggy code that finds its way into productive systems. Even the best programmers have lapses of concentration where they write typos like this (especially in open plan offices!).

The unit test which a programmer trying to achieve 100% coverage, would look something like this:

    @Test
    public void test() {
        assertEquals("a", new SomeClass().init(true, false).getResult());
        assertEquals("b", new SomeClass().init(false, true).getResult());
    }

Using Emma for Eclipse I measured complete coverage. But wait! There is still a bug. The code does not do what it says in the Javadoc spec. If I initialise the object with "true, true", then the result will be "ab", because of the "plus equals" operator. So even though I have 100% coverage, I still have a bug.

I asked myself what that means to me as a programmer. What do I need to look out for, when writing tests. Imagine the code above was tucked away among hundereds of other lines of code, then the chances of seeing it are really quite small. The test wouldn't be just two lines long and the problem wouldn't be jumping out of the page.

One way to look at the problem is to say that there is a bug because the code isn't fulfilling its contract. OK, I use "contract" in the loose sense of the word, but the Javadoc is basically a contract. It tells anyone calling that method what to expect, yet the codes is not doing what the user expects.

So perhaps one solution is to not only entirely exercise the code, but entirely exercise the contract? Is there a way to translate the Javadoc contract into something more concrete which the testing tools can help me check? Yes, namely my using some kind of Design (or Program) by Contract (DBC or PBC) framework. JSR-303 isn't strictly DBC, but close. It lets you use annotations to state your expectations about parameters passed to methods, as well as your expectation about the result being returned. You can create your own complex constraints quite easily. I added the following annotations to my code to help in my quest for bug free code:

    @Valid
    @NotNull
    @Size(min=0, max=1)
    public String getResult() {
        ...

Now, method validation (validating method calls, rather than validating the bean itself) is something which comes as an extra in Hibernate Validator, and which really isn't part of JSR-303 - it's only described in Appendix C as optional. To test this, I used Google Guice to add an AOP interceptor to any methods marked with the @Valid annotation, and in that interceptor, I call the Hibernate Method Validator. I end up with something like this:

.
.
.
    Injector injector = Guice.createInjector(new AbstractModule(){
        protected void configure() {
            bindInterceptor(Matchers.any(),
                    Matchers.annotatedWith(Valid.class),
                    new MethodValidatorInterceptor());
        }
    });
    SomeClass someClass = injector.getInstance(SomeClass.class);
    someClass.init(true, false);
    assertEquals("a", someClass.getResult());

    someClass = injector.getInstance(SomeClass.class);
    someClass.init(false, true);
    assertEquals("b", someClass.getResult());
.
.
.
public class MethodValidatorInterceptor<T> implements MethodInterceptor {

    public Object invoke(MethodInvocation invocation) throws Throwable {

        //validate all inputs
        Set<MethodConstraintViolation<T>> mcvs =
            methodValidator.validateAllParameters((T)invocation.getThis(),
               invocation.getMethod(), invocation.getArguments());
        if(mcvs.size() != 0){
            throw new IllegalArgumentException(String.valueOf(mcvs));
        }

        //call through to the actual method being called
        Object ret = invocation.proceed();

        //validate result
        mcvs = methodValidator.validateReturnValue((T)invocation.getThis(),
            invocation.getMethod(), ret);
        if(mcvs.size() != 0){
            throw new IllegalArgumentException(String.valueOf(mcvs));
        }

        return ret;
    }
}
.
.
.

The above is something which a (Java EE) container should do for me - I was just messing around with simple classes in a simple Java Project. Now, it isn't quite complete, because I still have 100% coverage, and I still have that bug, because the new annotations haven't really done anything useful. Well that isn't entirely true - the reader of the code knows that the contract is a little stricter than it was when it was simple Javadoc. The reader may assume that the system will check these constraints when the method is called. But while there is still a bug, I've laid the path for adding some full pre- or post-conditions. The next step was to add a new annotation, an interface and make use of them in the interceptor.

@Target( { ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface PreCondition {
    Class<? extends ConditionChecker> implementation();
}

/** Any pre- or post-condition is written in
  * an implementation of this interface
  */
public interface ConditionChecker {
    void checkCondition()
        throws ConstraintViolationException;
}

The annotation can be added to the business code, in this case to add a pre-condition. I created a similar annotation for post-conditions. When I add the pre-condition to a method, I also state which class contains the code for checking that pre-condition:

    @Valid
    @NotNull
    @Size(min=0, max=1)
    @PreCondition(implementation=MyPreAndPostCondition.class)
    public String getResult() {
        ...

The interceptor can check for the presence of such a pre-condition annotation before calling through to the method being called. If the annotation is found, the interceptor attempts to create an instance of the implementation class, and calls it's "checkCondition" method.

So the final part of this puzzle is to write a pre-condition which will help me to fail to get 100% coverage when I test with the test shown near the top of this post. Here it is, implemented as a static final inner class inside the SomeClass class, so that it has access to the fields "a" and "b":

public class MyPreAndPostCondition implements ConditionChecker {
    @Override
    public void checkCondition()
            throws ConstraintViolationException {

        //im going to list all combinations of
        //a and b which I support
        if(!a && b) return;
        if(!b && a) return;
        if(!b && !a) return;
        if(a && b) return;
    }
}

When I now test my code, I no longer get 100% coverage, because the last two lines in the pre-condition are not covered. Hooray! The tools can now tell me what I still need to test...

Now, while this technical solution works, I think it is really ugly. Like I said, if the code which is causing the problem were to be found within one or two hundred other lines of code, and were reliant on local variables rather than fields in my class, I would have no chance of using a pre-condition like this to help me locate the problem.

So to sum up, DBC isn't going to help me solve the problem that 100% code coverage can still contain errors. I think DBC frameworks (and there are a lot out there, some which do exactly what I have done here using the @PreCondition annotation) help to make contracts more concrete. If you use method validation from Hibernate Validator, you don't have to write as much Javadoc, because the reader knows that the container will give up before calling the method if anything fails to validate. To me as a programmer, that is much more satisfying than praying that some Javadoc reflects what I have really implemented.

I have known programmers who don't write tests because a DBC framework is in place and that makes them feel safe. But just because you declare a contract, does not mean the code actually works. Whether the code fails hard with a validation exception or at some time later because your code is buggy, makes no difference - both are inacceptable! From that perspective, DBC contracts are simply hints to the tester what could be useful tests, and they ensure that the code fails hard, early.

While I was refreshing my testing skills, I also learned the difference between mocks and stubs. For years I had always thought they were the same thing, but it turns out that stubs return pre-fed answers, whereby mocks check the sequence of calls to them too. On a different thread at DZone, someone made a point that unit testing was pointless because it never helped them find bugs, and it caused lots of work when refactoring, because all that does is break their tests. I'd say that this is simply a question of black box versus white box testing. Black box unit testing should never break if you refactor your code - the tests are simply clients to the code which you are refactoring, and tools like Eclipse will modify the calling code if you modify the interfaces being called, including tests. You can get pretty good testing results by just using black box tests - the large majority of the tests I write are black box tests and when I write such tests, I tend to have bug free code.

I've talked about writing contracts to help the reader determine what they should expect when they use your code. Unit tests themselves work similarly, because they show the reader examples of how to call your code and what to expect when your code changes system state. They hint to the reader how the writer intended the code to be used. While I don't advocate TDD (perhaps only because I have never been on a project which used it or at a company which valued quality enough to warrant TDD), I do encourage writing tests and using validation and pre-/post-conditions because they help document the code with the added bonus of finding the occassional bug. At the same time, I am an architect and we need to keep things like budgets in mind. You have an influence on your budget when you estimate, assuming you are allowed to do that. Your customer might push you to reduce your estimates, and that is an indication about their commitment to quality, because they won't budge on scope or delivery dates! So write as many tests as you can, within your budget and start with the code which is most complex and which gets called most often. and remember, 100% coverage is not really your best friend because bugs may still lurk.
 

From http://blog.maxant.co.uk/pebble/2011/11/28/1322465377854.html