Talk:Test for Required Behavior, not Incidental Behavior
From Programmer 97-things
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
In the version as of 2010-01-14, Kevlin Henney wrote:
> ... compareTo in Java's Comparable interface.
> Although the specific values -1 and +1 are commonly used in implementations to signify less than and greater than, respectively,
> programmers often mistakenly assume that these values represent the actual requirement and consequently
> write tests that nail this assumption up in public.
I think you need to make a distinction between ideal new code and trying to handle legacy code by adding new tests (according to actually assumed behaviour) for existing code.
In the latter situation I think it is reasonable to nail existing assumptions into test code, to avoid someone actually breaking existing code.
For example, regarding the Comparable example about using -1 and 1, assume you would find the following legacy code:
public class Executor {
public void doSomethingIfFirstObjectIsLarger(Object o1, Object o2) {
if(o1 instanceof Comparable && o2 instanceof Comparable) {
Comparable c1 = (Comparable) o1;
Comparable c2 = (Comparable) o2;
if(c1.compareTo(c2) == 1) {
doSomethingWithLargerObject(o1);
}
}
}
private void doSomethingWithLargerObject(Object o1) {
....
}
}
and also assume that you know for sure (e.g. through debugging) that one of those "Object" instances in some runtime scenario would be a Person class, with the folling implementation:
public class Person implements Comparable<Person> {
private String name;
public Person(String name) {
this.name = name;
}
public int compareTo(Person person) {
int compareToNumber = name.compareTo(person.name);
if(compareToNumber > 0) {
return 1;
}
else if(compareToNumber < 0) {
return -1;
}
else {
return 0;
}
}
...
[equals and hashCode implementations]
...
}
Then I think it would be reasonable to create a JUnit test, to actually nail this implementation into a new test (of course a new test since legacy code rarely have any existing tests):
public class PersonTest {
/**
* Please be VERY careful before changing this test code and the tested
* implementation of Person.compareTo, ro returning something else than 0, -1 or 1,
* because if you do change it, then you will __AT LEAST__ also have to change the
* following parts of the code, which relies on the fact that certain values are returned:
* @see [link to some legacy code you KNOW uses the assumption of -1, 1 or 0, e.g. the above mentioned method 'doSomethingIfFirstObjectIsLarger' ]
* @see [link to some more legacy code you KNOW uses the assumption of -1, 1 or 0]
* @see [link to some legacy code you KNOW uses other return values that -1 or 1, just to prevent certain code from being executed, e.g. see the Product class further down]
*/
@Test
public verify_Person_CompareTo_only_returns_zero_or_plusMinusOne() {
final String messageToDisplayIfTestFails = "There may now be a bug in a method " + Executor.class.getName();
final Person jonas = new Person("Jonas");
final Person tomas = new Person("Tomas");
// If you fail here, then please look out for a comparison like "if(c1.compareTo(c2) == 1) {" in the class pointed out by the error message you got.
// The Person class is known to have been used as an "Object" parameter to that method, and the existing code assumed 'compareTo' to return 1
// if the instance was larger than the parameter, so if the Person class no longer returns 1 (if this test below fails)
// then it might be an indication of having introduced a bug when Person.compareTo was changed
assertEquals(messageToDisplayIfTestFails, 1, tomas.compareTo(jonas));
assertEquals(messageToDisplayIfTestFails, -1, jonas.compareTo(tomas));
assertEquals(0, jonas.compareTo(jonas));
}
I would then also consider make a comment about it in the implementation method, kind of like this:
public class Person implements Comparable<Person> {
...
/**
* @return 0, -1 or 1 (even though the method contract in the interface allows other values)
* and please be VERY careful about changing this implementation to simply return 'name.compareTo(person.name);'
* (see further comments at the JUnit test for this method)
*/
public int compareTo(Person person) {
...
}
...
}
Of course, you may argue that all code with the wrong usage of the Comparable interface should be fixed instead, e.g. by changing the above "if(c1.compareTo(c2) == 1) {" into "if(c1.compareTo(c2) > 0) {" but it may be difficult in a big legacy application to dare doing these kind of changes, because then it would not be too unlikely that other things would start falling apart in an application with bad legacy code which typically has virtually non-existing regression tests.
If you, the reader, do not have enough imagination (potentially as a result of not having been exposed to enough legacy code) to understand how the above mentioned change (from "==1" to "> 0" ) might introduce new incorrect behaviour, then I would like to inform you that in a true legacy application you can expect any kind of bad code to exist.
For example, the above method ('doSomethingIfFirstObjectIsLarger') with some 'Object' parameters may become invoked through reflection code and thus hard to figure out ALL places from where it is used (though, by debugging one particular use case scenario, you will find one stacktrace showing you from where the invocation came, but there might be lots of other potential execution paths as well, except the one(s) you might be aware of).
Therefore (during the lifespan of the old legacy application) some developer may have created an ugly workaround to prevent the behaviour for the "larger object" above to become executed, by abusing the compareTo method and letting it return 2 instead of 1.
In other words, a class such as below might exist:
public class Product implements Comparable<Product> {
private Long productId;
public Product(long productId) {
this.productId = productId;
}
/**
* Please note that if you change the implementation to return other values than 2 or -2
* (such as returning 1 and -1 instead, or returning any positive or any negative number as in the actual method contract)
* then the method implementation would indeed continue to work for sorting products by using a method
* invocation such as 'java.util.Collections.sort(List<Product> products)' , but it may instead break other parts of the
* code where certain assumptions has been made about the return values.
* For example, there are situations when instances of this class are becoming parameters (through reflection code) into a method
* where the return value of this method is compared with 1 or -1 to execute certain code, and to avoid that code to become executed for
* this Product class, we instead return 2 or -2 as a way to avoiding that kind of execution to occur.
* @return 0, 2 or -2, depending on whether the 'this instance' is equal to, or larger than, or less than the parameter instance
*/
public int compareTo(Product product) {
int compareToNumber = productId.compareTo(product.productId);
if(compareToNumber > 0) {
// (the return values used here is an ugly way of preventing certain code from becoming executed)
return 2; // ( avoids "if(c1.compareTo(c2) == 1)" to become true, but would not prevent "if(c1.compareTo(c2) > 0)" from becoming true... )
}
else if(compareToNumber < 0) {
return -2;
}
else {
return 0;
}
}
...
}
Now I also think it would be reasonable to create a similar ProductTest method similar to the PersonTest method above, but this time asserting the value 2 instead of assering the value 1.
However, as an alternative to asserting Person.compareTo to return 1 and asserting Product.compareTo to return 2, you may instead choose to write a better test for the legacy code, which tests the actually desired behaviour above, i.e. the dirty workaround code for avoiding some code to become executed when the 'Object' typed instances are 'Product' instances.
This could be done by refactoring the Executor class by introducing an interface that can be mocked from a test class, like this:
public interface ExecutionHandler {
void doSomethingWithLargerObject(Object o1);
}
public final class Executor {
private final ExecutionHandler executionHandler;
Executor(ExecutionHandler executionHandler) {
this.executionHandler = executionHandler;
}
Executor() {
this(executionHandlerDefaultLegacyImplementation);
}
public void doSomethingIfFirstObjectIsLarger(Object o1, Object o2) {
if(o1 instanceof Comparable && o2 instanceof Comparable) {
Comparable c1 = (Comparable) o1;
Comparable c2 = (Comparable) o2;
if(c1.compareTo(c2) == 1) {
executionHandler.doSomethingWithLargerObject(o1);
}
}
}
private final static ExecutionHandler executionHandlerDefaultLegacyImplementation = new ExecutionHandlerDefaultLegacyImplementation();
private static final class ExecutionHandlerDefaultLegacyImplementation implements ExecutionHandler {
public void doSomethingWithLargerObject(Object o1) {
....
}
}
}
The above class has the very same behaviour as the original Executor class further up, but now has been provided with an interface that can be constructor injected from a test class as below.
The test code could now be written something like this:
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import org.junit.Test;
public class ExecutorTest {
@Test
public void verifyingBehaviourForSomeClassesKnownToBeUsedAsObjectParameters() {
final Person jonas = new Person("Jonas");
final Person tomas = new Person("Tomas");
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(1, tomas, jonas);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, jonas, tomas);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, tomas, tomas);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, jonas, jonas);
// the test below would fail if Product.compareTo would return 1 instead of 2 when the instance is larger
final Product product_1 = new Product(1);
final Product product_2 = new Product(2);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, product_1, product_2);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, product_2, product_1);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, product_1, product_1);
verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(0, product_2, product_2);
// Please note that I (Tomas) did not write the above tested legacy code, but I do know for sure that the above tested method
// is indeed being used with at least 'Person' and 'Product' instances with the desired behaviour as above, for triggering certain code
// to become executed for Person, when one person is considered larger according to compareTo method,
// while the same kind of behaviour should not become triggered for the product class.
// Regarding all other "Object" instances that potentially might become used as parameters and the desired behaviour through compareTo method, I simply
// do not know about that kind of behaviour, but I have tried to do my best for at least implementing regression tests for the behaviour
// that I have been able to understand...
}
private void verifyExpectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked(
final int expectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked,
final Object firstObject,
final Object secondObject
) {
final ExecutionHandler executionHandler = mock(ExecutionHandler.class);
final Executor executor = new Executor(executionHandler);
executor.doSomethingIfFirstObjectIsLarger(firstObject, secondObject);
verify(
executionHandler,
times(expectedNumberOfTimesTheMethodDoingSomethingWithLargerObjectShouldBecomeInvoked)
).doSomethingWithLargerObject(firstObject);
}
}
The above test would fail if Person.compareTo returns another value than 1 when the instance is considered larger, and it would also fail if Product.compareTo returns another value than 2 when that instance is considered larger.
So, it would kind of test the same thing as directly testing the compareTo values, in the sense that the code in its current state would trigger both kind of tests to fail.
However, this latter test (with the 'Test Spy' framework 'Mockito') would be cleaner in the sense that it probably tests what the legacy developers intended, i.e. they intended at least that certain code would become triggered for the larger of two Person instances, while similar kind of code should not become triggered for the larger of two Product instances (when some common, but ugly, reflection code using type 'Object' would be used for Person and Product and potentially other kind of classes).
The legacy developers did hopefully not intend to return 1 for some classes 'compareTo' and return 2 for some other classes just for the purpose of abusing the contract of the Comparable interface, but they probably did it just as a bad implementation solution for sometimes triggering some code and to sometimes avoiding it to become triggered.
Though, being realistic, I would not really expect an averaged skilled developer to be able to figure out that he could do this kind of refactoring and using a Test Double for verifying the desired behaviour in the legacy code.
I think it would be more realistic to hope that when an average developer would see that the method "public void doSomethingIfFirstObjectIsLarger(Object o1, Object o2) {" in runtime can receive Person or Product instances (for example) as parameters, and make a comparison of the 'compareTo' value to being exactly "1" or exactly "2", then such a developer might have a look in the Person and Product implementations to verify that indeed the exact values 1 or 2 are returned there, and then he might think that it is important to preserve that behaviour for avoiding the code to break, and then he might write a PersonTest (and ProductTest class) as I did further up.
I think this would at least be better than doing nothing, i.e. would be better to write a simple test that would hopefully prevent the code from breaking, by having an assertion failure if one of those two compareTo methods would become changed.
/ Tomas Johansson
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
