Refactoring Tests
An attitude that can be very detrimental to a test-driven development process is to consider the tests as sacrosanct, a sacred specification of the system that once written can never be changed. If a team do change the tests they do not improve them as they learn more about the application domain, the technologies they are using and the practices of test-driven development itself. As a result, the tests become confusing to read and hard to diagnose when they fail. Eventually the team find the tests so hard to work with that they are tempted to remove them from the build altogether. This situation is made worse when one part of the organisation (QA, for example) considers that it "owns" the tests but expects another part of the organisation (development) to diagnose and fix test failures.
I find it very helpful to refactor tests very frequently while I'm developing. I use the tests as a record of what my pair and I currently understand about the functionality we're working on. When working on something I don't understand well I will often refactor a every few minutes or even every few seconds to jot down of what I have learned, usually renaming identifiers or pulling out methods and variables to introduce explanatory names.
To show what I mean, here are the changes that were recorded in my Eclipse local history for just one test in jMock 2 as I analysed and fixed a defect (warning: low-level hackery ahead). Steve Gilbert, who reported the defect, helpfully included a JUnit 4 test case that demonstrated the failure. The first thing I did was add the test to the jMock 2 test suite and convert it to JUnit 3 like the rest of the jMock tests.
public class ComparatorTest extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testFailingTest() throws Exception {
final MyComparator comparator = context.mock(MyComparator.class);
final MyComparable a = new MyComparable();
final MyComparable b = new MyComparable();
context.checking(new Expectations() {{
one(comparator).compare(a, b); will(returnValue(0));
}});
compare(a, b, comparator);
}
public void testPassingTest() throws Exception {
final Comparator<MyComparable> comparator = context.mock(MyComparator.class);
final MyComparable a = new MyComparable();
final MyComparable b = new MyComparable();
context.checking(new Expectations() {{
one(comparator).compare(a, b); will(returnValue(0));
}});
compare(a, b, comparator);
}
private <T> int compare(T a, T b, Comparator<T> c) {
return c.compare(a, b);
}
static class MyComparable implements Comparable {
public int compareTo(Object o) {
return 0;
}
}
static class MyComparator implements Comparator<MyComparable> {
public int compare(MyComparable o1, MyComparable o2) {
return 0;
}
}
}
I suspected that the bug had nothing to do with Comparators and Comparables. I explored that possibility by using strings instead of MyComparable objects to see if anything changed.
public class ComparatorTest extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testFailingTest() throws Exception {
final MyComparator comparator = context.mock(MyComparator.class);
context.checking(new Expectations() {{
one(comparator).compare("a", "b"); will(returnValue(0));
}});
compare("a", "b", comparator);
}
public void testPassingTest() throws Exception {
final Comparator<String> comparator = context.mock(MyComparator.class);
context.checking(new Expectations() {{
one(comparator).compare("a", "b"); will(returnValue(0));
}});
compare("a", "b", comparator);
}
private <T> int compare(T a, T b, Comparator<T> c) {
return c.compare(a, b);
}
public static class MyComparator implements Comparator<String> {
public int compare(String s1, String s2) {
return 0;
}
}
}
The test still failed in the same way. Comparables had nothing
to do with the problem so I introduced some new types into the test
to better describe what was really being exercised. I also renamed
the compare method to useTheMock for the
same reason.
public class ComparatorTest extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testFailingTest() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
public void testPassingTest() throws Exception {
final AnInterface mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
private void useTheMock(AnInterface mock, String string) {
mock.doSomethingWith(string);
}
public interface AnInterface {
void doSomethingWith(String arg);
}
public static class AnImplementation implements AnInterface {
public void doSomethingWith(String arg) {
}
}
}
Both tests passed! I didn't expect that. The only significant difference between the AnInterface type I had defined and Comparable is that AnInterface is no longer generic. I made it generic to see what would happen.
public class ComparatorTest extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testFailingTest() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
public void testPassingTest() throws Exception {
final AnInterface<String> mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
private <T> void useTheMock(AnInterface<T> mock, T arg) {
mock.doSomethingWith(arg);
}
public interface AnInterface<T> {
void doSomethingWith(T arg);
}
public static class AnImplementation implements AnInterface<String> {
public void doSomethingWith(String arg) {
}
}
}
That made the test fail again. That showed me that the bug was something to do with non-generic classes implementating generic interfaces, so I renamed the test to record what I'd learned.
Originally the test methods were well named because they accompanied to the bug report: one test passed and the other failed unexpectedly. Now they were integrated into the jMock test suite those names no longer make sense. After all, when I finished fixing the bug they should both be passing. The difference between the two tests seemed to be that the one is calling the mock through the mocked concrete class, not through the interface, so I renamed them to reflect that.
public class MockingImplementationOfGenericTypeAcceptanceTests extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testWhenInvokedThroughClass() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
public void testWhenInvokedThroughInterface() throws Exception {
final AnInterface<String> mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
useTheMock(mock, "a");
}
private <T> void useTheMock(AnInterface<T> mock, T arg) {
mock.doSomethingWith(arg);
}
public interface AnInterface<T> {
void doSomethingWith(T arg);
}
public static class AnImplementation implements AnInterface<String> {
public void doSomethingWith(String arg) {
}
}
}
Tracing through with the debugger, it appears that the problem arises when the expectation is set up through the concrete class but the method is actually invoked through the interface. Our passing test sets up expectations and invokes mocked methods through the interface. The failing test sets up expectations through the class and invokes mocked methods through interface. What about the third case: when a test sets up and invokes through the class? I renamed the methods again to record what I'd learned and added the additional case to see what happened.
After that refactoring the useTheMock method was
pointless because it was only only used in one test and also made
the tests harder to read, so I inlined it.
public class MockingImplementationOfGenericTypeAcceptanceTests extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testWhenDefinedAndInvokedThroughClass() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
mock.doSomethingWith("a");
}
public void testWhenDefinedThroughClassAndInvokedThroughInterface() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
((AnInterface<T>)mock).doSomethingWith("a");
}
public void testWhenDefinedAndInvokedThroughInterface() throws Exception {
final AnInterface<String> mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
mock.doSomethingWith("a");
}
public interface AnInterface<T> {
void doSomethingWith(T arg);
}
public static class AnImplementation implements AnInterface<String> {
public void doSomethingWith(String arg) {
}
}
}
Digging around with the debugger some more I discovered that
when a method of a generic interface is invoked through the
interface but implemented by a non-generic class, the
invocation gets routed through a "bridge
method" that implements the downcast. Bridge methods are
not visible at compile time. They are an implementation detail
generated by the compiler and only visible at runtime. Luckily that
information is exposed through the reflection API; Method objects
have an isBridge property. I added a comment to the
test to explain
why the test fails. (Note: I've removed foul
language about Java generics from the comment to protect those with
tender sensibilities).
public class MockingImplementationOfGenericTypeAcceptanceTests extends TestCase {
private Mockery context = new JUnit4Mockery() {{
setImposteriser(ClassImposteriser.INSTANCE);
}};
public void testWhenDefinedAndInvokedThroughClass() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
mock.doSomethingWith("a");
}
public void testWhenDefinedThroughClassAndInvokedThroughMethod() throws Exception {
final AnImplementation mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
// Note: this is invoked through a "bridge" method and so the method
// invoked when expectations are checked appears to be different from
// that invoked when expectations are captured.
((AnInterface<String>)mock).doSomethingWith("a");
}
public void testWhenDefinedAndInvokedThroughInterface() throws Exception {
final AnInterface<String> mock = context.mock(AnImplementation.class);
context.checking(new Expectations() {{
one(mock).doSomethingWith("a");
}});
mock.doSomethingWith("a");
}
public interface AnInterface<T> {
void doSomethingWith(T arg);
}
public static class AnImplementation implements AnInterface<String> {
public void doSomethingWith(String arg) {
}
}
}
Now I could fix the bug. I changed the ClassImposteriser to not intercept bridge methods but let them forward the invocation on to the concrete implementation. This way expectations specified via bridged or non-bridged methods can be satisfied by invocations of bridged or non-bridged methods.