"Untestable" code - design vs. testability trade-off
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:
- new methods are neither
private
norfinal
- 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:
- subclass
ServiceA
class and overwrite the extracted methods - 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
- No aspectj-introspection-reflection-mock-groovy magic. Fellow developers have good chances of understanding what's going on here.
- Test method (method:
testBusinessOperation()
, not the whole test-class !) is very easy to understand. - No problems with maven or code coverage.
Cons
- Won't work with
final
keyword as we must create a subclass of the tested class. - Even after dropping
final
keyword it's impossible to:- check if method
save
orfind
was ever executed onDatabase
class - if method
computeTotal
was ever executed on object ofServiceB
class
- check if method
- Requires significant refactorings in tested class. Imagine what happens if there are more static calls / constructors used in tested class.
- Weakens tested class by removing "final" and thus allowing inheritance.
- 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:
uc-design-one-final.zip
contains code as explained in this text (withfinal
keyword removed fromServiceA
class)uc-design-no-final.zip
contains a slightly changed version, wherefinal
keyword is removed from two classes:ServiceA
andServiceB
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.
Attachment | Size |
---|---|
uc-design-one-final.zip | 10.95 KB |
uc-design-no-final.zip | 100.16 KB |
This used to be my blog. I moved to http://tomek.kaczanowscy.pl long time ago.
thank you
thank you