Test Everything, No Matter How Simple
There are many reasons to write your tests first. An oft unmentioned benefit is that it makes the programmer stop and think about what they’re doing. More often than you’d expect, the obvious fix is not the right answer; and writing the test first can reveal that.
For example, recently Wolfgang Hoschek pointed out that in XOM the Attribute class’s setType() method was failing to check for null. Once he pointed it out, it was an obvious problem so I opened up Eclipse and got ready to fix it:
private void _setType(Type type) {
this.type = type;
}The fix was trivial:
private void _setType(Type type) {
if (type == null) {
throw new NullPointerException("Null attribute type");
}
this.type = type;
}Fortunately I stopped myself. Even with a known and obvious bug, one shouldn’t fix it without first writing a test case, so I did:
public void testNullType() {
try {
a1.setType(null);
fail("Didn't throw NullPointerException");
}
catch (NullPointerException success) {
assertNotNull(success.getMessage());
}
}And it’s a good thing I did to because it was only when writing the test case that I realized I was patching the wrong method. The check belonged in setType(), not _setType(). The former is the public method. The latter is a package-access method for internal use that saves time by doing less checking in situations where the data is already known to be good. Had I written the code without a test case, I wouldn’t have noticed that I was slowing down XOM by putting the check in the wrong place.
Even if I hadn’t been about to make the wrong fix, it still would have made sense to write the test. The test is not just about writing the code now. It’s about documenting the proper behavior of the code for future programmers. Once the test is in the test suite, it’s clearly documented that throwing a NullPointerException when the argument is null is the expected and contracted behavior. A future developer, whether me or someone else, can still change it if they want to, of course. However, they’d have to make a deliberate and carefully considered decision to do so. it won’t be changed by accident or oversight. It is unlikely to become a regression.
No code should be untested. Even the simplest code (like this almost trivial setter method) can have bugs, so even the simplest code needs to be tested. No exceptions! No code is too simple to fail.
August 22nd, 2006 at 2:00 pm
It seems to me the bug is in setType() and not in _setType(), so I would continue to test setType(), as you have done here, and not to test _setType(), as you have done here. In t his case, _setType() remains Too Simple to Break, and indeed, is not broken.
August 23rd, 2006 at 7:04 am
You’re not quite understanding the problem. Indeed
_setType()should not be tested directly, because it is not public API. The test should go through thesetType()method instead. That was never the issue.My point is that my initial “fix” of changing the
_setType()method was incorrect, and writing the test method first helped me see that. When I developed the fix just by looking at the code, I didn’t notice that. Viewing the code from the perspective of a client class, rather than the model class, helped me see things differently.September 10th, 2006 at 12:31 am
I assure you I definitely understand the issue. I simply don’t understand your reasoning. Let me try to be very clear. As I read it, you state the following:
1. I found a bug in some code.
2. When I went to fix the bug, I first did it before writing a test, then I stopped myself and decided to write the test.
3. It was in writing the test that I realized I tried to fix the bug in the wrong place.
Therefore, test all code, no matter how simple.
It’s that last step that doesn’t sit right with me. It seems like a sideways jump, rather than a step.
I agree that when we find a bug, we should write a test to expose the bug before fixing it. Without that, how do we know we fixed it? Even so, I think you conflate two lines of reasoning and use a conclusion from one line to justify a statement in the other.
One line of reasoning regards not fixing bugs until there is a test that fails because of the bug.
Another line of reasoning regards the ROI on testing code that I feel is Too Simple to Break. You think the R is worth the I.
The way I read it, you have concluded that it is worth testing Too Simple to Break code, because a test helped you find and fix a bug in code that wasn’t Too Simple to Break: namely setType(). If setType() is supposed to do more than just this.type = type, then it is Complex Enough to Break, and so should absolutely be tested.
So you seem to have used an example of finding a bug in code that is Complex Enough to Break to argue that one should test absolutely everything, including code that is Too Simple to Break. I just don’t understand how that reasoning works. I do certainly agree with the notion that by writing a test, you slowed down, thought about what you were doing, then realized you were about to fix the wrong code, but that’s generally a good idea anyway.
September 28th, 2006 at 2:55 am
Not to be nitpicking – but shouldn’t you throw an IllegalArgumentException instead of a NullPointerException?
From the J2SE API on IllegalArgumentException
“Thrown to indicate that a method has been passed an illegal or inappropriate argument.”
September 28th, 2006 at 6:19 am
No.
NullPointerExceptionis a special case. See Effective Java, Item 23 and Item 42. From p. 176, “If a caller passesnullin some parameter for which null values are prohibited, convention dictates thatNullPointerExceptionbe thrown rather thanillegalArgumentException.”