Printer-friendly version

This is a part of "Untestable code" series. See the introduction to know what is it all about (yes, you really should go there, do it).

The main idea of the series is to write unit-tests for a particularly nasty piece of code. This part will show how we can make testing possible thanks to some design sacrifices.

Please notice the source code attached at the end of this text.

The idea is quite simple: if it's so hard to test a particular class than maybe some refactorings (like extract method) combined with subclassing will help ? Let's see.

Original class

This is the class under test:

public final class ServiceA {

    public void doBusinessOperationXyz(EntityX data) 
        throws InvalidItemStatus {

        List items = Database.find(
            "select item from EntityY item where item.someProperty=?",
            data.getSomeProperty());
        BigDecimal total = new ServiceB().computeTotal(items);
        data.setTotal(total);
        Database.save(data);
    }

}

Changing the original class

The problematic lines are these containing constructor call and static method invocations. Let's extract all these lines into separate methods:

public class ServiceA {
    public void doBusinessOperationXyz(EntityX data) 
            throws InvalidItemStatus {
        List items = find(data);
        BigDecimal total = computeTotal(items);
        data.setTotal(total);
        save(data);
    }

    void save(EntityX data) {
        Database.save(data);
    }

    BigDecimal computeTotal(List items) throws InvalidItemStatus {
        return new ServiceB().computeTotal(items);
    }

    List find(EntityX data) {
        return Database.find(
            "select item from EntityY item 
                where item.someProperty=?", data.getSomeProperty());
    }
}

Two things to notice here:

1. new methods are neither private nor final
2. class ServiceA is no longer final

The reason for this is that we need to subclass ServiceA and overwrite these methods during testing.

BTW: one can argue that this is not a real "extract method" refactoring. And I'd agree with this opinion. We do the same thing (changing few lines into separate method) but for completely different reasons.

Test class

ServiceA class is now ready to be tested, so let's do it.

The idea is like this:

1. subclass ServiceA class and overwrite the extracted methods
2. test subclassed version of ServiceA

public class ServiceATest extends TestCase {

    private static final BigDecimal TOTAL = BigDecimal.TEN;

    class MyServiceA extends ServiceA {
        private boolean saved = false;

        @Override
        BigDecimal computeTotal(List items) throws InvalidItemStatus {
            return TOTAL;
	}

        @Override
        List find(EntityX data) {
            return Collections.EMPTY_LIST;
        }

        @Override
        void save(EntityX data) {
            this.saved = true;
        }

        public boolean wasSaved() {
            return saved;
        }
        
    };

    /** Test spy. */
    private MyServiceA sA = new MyServiceA();

    public void testBusinessOperation() throws InvalidItemStatus {
        EntityX entity = new EntityX(1, "abc", "def");
        sA.doBusinessOperationXyz(entity);
        assertEquals(TOTAL, entity.getTotal());
        assertTrue(sA.wasSaved());
    }
}

Ok, now what has happened here ? All the methods extracted during refactoring are overwritten. I use Test Spy to check if data was saved. As for the rest of methods I provide some dummy values.

In fact, we can't test if tested class sends any messages to it's collaborators, as this is exactly what we have just "removed" from our subclass. This means, we can't check if Database.save() was ever executed. Same goes to computeTotal() method of ServiceB class. See the code coverage report to check what really got tested.

Pros and Cons

Pros

1. No aspectj-introspection-reflection-mock-groovy magic. Fellow developers have good chances of understanding what's going on here.
2. Test method (method: testBusinessOperation(), not the whole test-class !) is very easy to understand.
3. No problems with maven or code coverage.

Cons

1. Won't work with final keyword as we must create a subclass of the tested class.
2. Even after dropping final keyword it's impossible to:
   • check if method save or find was ever executed on Database class
   • if method computeTotal was ever executed on object of ServiceB class
3. Requires significant refactorings in tested class. Imagine what happens if there are more static calls / constructors used in tested class.
4. Weakens tested class by removing "final" and thus allowing inheritance.
5. Test class is much bigger than it should be. It contains a lot of fixture-setup code.

My 3 cents

I use this technique only if there is one or maybe two constructor/static calls. It requires a lot of rather awkward changes to the original code and test methods are quite big.

One more final to go

If we decide to remove final not only from ServiceA class but also from ServiceB then it's possible to check one more thing: if method "computeTotal" was ever executed on object of ServiceB class.

ServiceA class

In ServiceA this line:

BigDecimal total = computeTotal(items);

turns into this:

ServiceB serviceB = getServiceB();
BigDecimal total = serviceB.computeTotal(items);

Test class has to change as well, because now we need to provide our version of ServiceB (as subclass or mock). That is the price we pay for being able to test more code than before. See attached code for details.

Attachments

The files are at the end of this text.

There are two attachments:

1. uc-design-one-final.zip contains code as explained in this text (with final keyword removed from ServiceA class)
2. uc-design-no-final.zip contains a slightly changed version, where final keyword is removed from two classes: ServiceA and ServiceB

Run both of them by typing mvn test site. Results of tests will be printed directly to the console. Code coverage report is available in target/site directory (see index.html in there).

Links

All links gathered here.