Catch all
try {
callFailingMethod()
fail()
} catch (Exception e) {
}
Problems:
When you look at the test code you cannot see which type of exception is thrown. First it is better for clarity to document which type is thrown and second any bugs in the called code who throw unintended exceptions are swallowed here.
Better:
try {
callFailingMethod()
fail()
} catch (ParseException e) {
}
Problems:
If it fails you don’t see why: so always use a message for fail.
Better:
try {
callFailingMethod()
fail('ParseException expected')
} catch (ParseException e) {
}
Problems:
If an exception is thrown, you don’t assert that it is the expected exception, so test for the exception message.
Solution:
try {
callFailingMethod()
fail('ParseException expected')
} catch (ParseException e) {
assertEquals("Invalid character at line 2", e.getMessage())
}
Using assert
assert isOdd(3)
Problems:
If you do not enable assertions on the JVM (by passing -ea) this line does nothing and the test passes fine every time.
Better:
assertTrue(isOdd(3))
Problems:
If assertTrue or assertFalse fails, you just get a generic error message, better use a message which communicates the error/
Solution:
assertTrue("3 should be odd", isOdd(3))
AssertTrue instead of assertEquals
assertTrue('Expected: 1+2 = 3', sum(1, 2) == 3)
Problems:
You don’t see the actual value here, you could include it in the message, but there is an assertion for that: assertEquals
Solution:
assertEquals(3, sum(1, 2))
Conditional logic in tests
if (isOdd(value)) {
assertEquals(5, calculate(value))
} else {
assertEquals(6, calculate(value))
}
Problems:
Can you look at the test source code and tell me which branch is used? If only one is used all the time, erase the other. If both are used, first make the test deterministic and use two tests, one for each branch.
Schneide Blog 
ExpectedException rule is interesting to solve the first code smells.
For assertions you can take a look at fest or harmcrest.
The assertion error messages are really clearer and meaning full and always up to date !
My testing toolbox
In JUnit, you could use an ExpectedException to test whether the expected exception has been thrown (see http://kentbeck.github.com/junit/javadoc/4.10/org/junit/rules/ExpectedException.html ) It looks much clearer to me than calling fail.
Yes, thank you.
The patterns where extracted from (old) JUnit3 tests, so the solution could be made even better when using JUnit4.
Better would be to use JUnit 4.x instead of JUnit3.x, and use the @Test(expected=MyException.class) annotation. So,
@Test(expected=ParseException.class)
public void phoneNumbersDoNotHaveLetters()
throws ParseException {
parser.parse(“ABC-DEFG”);
}
This will fail unless a ParseExcpetion is thrown.
If you want to test the message itself, use the latest version of JUnit, and use the ExpectedException @Rule. See http://kentbeck.github.com/junit/javadoc/latest/org/junit/rules/ExpectedException.html.
Assert.assertEquals is not enough. Suppose you have a class that contains a mutable object:
public class DateRange {
private Date start;
private Date end;
//constructors, getters, setters elided.
}
Then you need tests that enforce that you do not alias its values.
@Test
public void settingDateSets() {
DateRange range = new DateRange(…..);
Date now = new Date();
range.setStart(now);
Assert.assertEquals(“Start date”, now, range.getStart());
}
and also
@Test
public void settingDateDoesNotAlias() {
DateRange range = new DateRange(…..);
Date now = new Date();
range.setStart(now);
Assert.assertNotSame(“Start date”, now, range.getStart());
}
Otherwise somebody could create a date range, and modify the start or end objects outside its control.
If I understand you correctly, you mean that a setter should make a copy of its passed value and the test should test for this. That’s another good aspect. Thank you
Ye. It’s one of the rules from “Effective Java”. Classes with mutable instance variables should copy them both on input and on output. Otherwise a malicious programmer could break your objects. Suppose DtaeRange had vanilla getters and setters. Consider code like
Date today = new Date();
Date tomorrow = …; // code omitted.
DateRange range = new DateRange(today, tomorrow);
// Assume DateRange checks that start is before end.
today.setTime(aLongRepresentingNextYear);
//Since DateRange did not copy start,
// range.start is now after range.end.
As Effective Java said, don’t use clone() to copy the dates on input either. Somebody might give a preverse subclass of Date. In fact try to read and memorize that book.
Best is to use immutable objects as much as possible. Use the Joda-Time library instead of java.util.Date, for example. Use the offical replacement Date library when Java 8 or 9 eventually adds it
Yes, we use Joda Time for our projects since it handles many things much better than the built in Date/Calendar libs. The above code is just used Date as an example to not complicate it.
I agree with you that immutable objects are the way to go, it’s one of the things which I find very interesting in languages like Clojure which have builtin immutable or value objects. I’ve read Effective Java but yes, I should re-read it and should adhere more to the rules in there. Immutable objects make using them easier especially in multithreaded environments. Maybe you’ve read it already but I found John Carmack’s approach to using functional programming parts very pragmatic and helpful: http://www.altdevblogaday.com/2012/04/26/functional-programming-in-c/
Thanks for this nice article on some antipatterns of unit tests, but I think there are two little mistakes in the last solution of the exception example:
try {
callFailingMethod()
} catch (ParseException e) {
fail(“Invalid character at line 2”, e.getMessage())
}
The first one is, that you use the “fail” method to compare the two messages. I think this should be an “asserEquals” call, shouldn’t it?
So I think it should be like this:
try {
callFailingMethod()
} catch (ParseException e) {
assertEquals(“Invalid character at line 2”, e.getMessage())
}
The other one is, a bit difficult because I do not know if I understand the code correct.
In my opinion you want to test that the “callFailingMethod” throws an exception. But your test will also pass if no exception is thrown. For this case the “fail” call after the “callFailingMethod” is necessary and can not be removed.
So my final solution looks like this:
try {
callFailingMethod()
fail(“ParseException should be thrown.”)
} catch (ParseException e) {
assertEquals(“Invalid character at line 2”, e.getMessage())
}
Am I right or do I miss something?
You are right, thank you, missed out on that one, Updated the post accordingly.
So… in summary: Always use a message. Enable assertions. Don’t have multiple code paths in a test.
Yep
I am not sure about always using a message for each assertion…
In practice, it is hardly useful because you still need to open the test method in your IDE, run it to confirm the failure, apply a fix, then run the test again to confirm it now passes. Just reading the error message (plus the test name) is usually not enough to understand what the test is about or why it failed. You actually need to look at the test as a whole to achieve that, and having that message or not will often not make any real difference. Not to mention that such messages can easily be poorly written or misleading. At least, this has been my experience.