Concept question about JUnit Failures

[Rhino]

I'm having a bit of a conceptual struggle right now with the right way to
handle exceptions in JUnit tests. Since I always explain concepts better in
terms of examples, let me describe a scenario to you which should
illustrate the situation clearly.

I have a method called getRGBColor() whose input parameter is a six letter
String that is supposed to contain exactly six hex digits. Those hex digits
are the String representation of an RGB Color. Therefore, "FFFFFF"
represents White.

I have no trouble writing a JUnit test that passes in "FFFFFF" as an input
value and then verifies that the Color returned by the method is White i.e.
Color(255,255,255) so I can test that aspect of the code without trouble.

However, the method throws IllegalArgumentException if the input String is
not precisely 6 characters long or if any of the characters are not hex
digits.

I can easily invoke the method with a value that will cause the exception
to be thrown and I can catch it with a standard try/catch block. I can also
put code in my catch block to make sure that JUnit reports the error from
the Exception in the method being tested. I end up with this test which
correctly reports the IllegalArgumentException within the JUnit window:

try {
Color actualRGBColor = colorConversionUtils.getRGBColor("FFFFFFA");
Color expectedRGBColor = new Color(255, 255, 255);
assertTrue("Actual color, " + actualRGBColor + ", does not equal expected
color, " + expectedRGBColor, actualRGBColor.equals(expectedRGBColor));

}
catch (IllegalArgumentException ia_excp) {
assertionFailedError = new AssertionFailedError(ia_excp.getMessage());
assertionFailedError.initCause(ia_excp);
throw assertionFailedError;
}


So far, so good. Now, here's where my problem comes in. When I execute this
test, the method in which it is situated inevitably results in a black X
decoration on the test result in the JUnit window of Eclipse. (I'm using
JUnit3 for now.) I know that's reasonable given that it is correctly
reporting an exception thrown by a method that is under test.

However, I had the distinct impression in a conversion some months ago,
that a properly written set of JUnit tests should always give me a green
checkmark decoration on every test. And that makes a lot of sense to me
too. Ideally, given that unit tests may be executed hundreds of times and
each test class may have dozens of test methods in it, the last thing
anyone wants to do is have to inspect each test method individually and
know exactly which ones should have had green checkmarks, black X's and red
X's. It's simply far easier to make sure that all of the tests had green
checkmarks and only dive into them if some test _doesn't_ have a green
checkmark.

I don't see how I can reconcile these two ideas. If I test for thrown
exceptions, I will inevitably get some black X's. (Unless there is some way
to negate the exception test so that it only gives the black X if the
exception FAILS to be thrown???) But if I get black X's, how am I to know
quickly which of the test methods SHOULD be getting black X's and which
ones should be getting green checkmarks? I don't much like the idea of
trying to document all that and I certainly can't memorize it all....

 

[Daniel Pitts]

I'm having a bit of a conceptual struggle right now with the right way to
handle exceptions in JUnit tests. Since I always explain concepts better in
terms of examples, let me describe a scenario to you which should
illustrate the situation clearly.

I have a method called getRGBColor() whose input parameter is a six letter
String that is supposed to contain exactly six hex digits. Those hex digits
are the String representation of an RGB Color. Therefore, "FFFFFF"
represents White.

I have no trouble writing a JUnit test that passes in "FFFFFF" as an input
value and then verifies that the Color returned by the method is White i.e.
Color(255,255,255) so I can test that aspect of the code without trouble.

However, the method throws IllegalArgumentException if the input String is
not precisely 6 characters long or if any of the characters are not hex
digits.

I can easily invoke the method with a value that will cause the exception
to be thrown and I can catch it with a standard try/catch block. I can also
put code in my catch block to make sure that JUnit reports the error from
the Exception in the method being tested. I end up with this test which
correctly reports the IllegalArgumentException within the JUnit window:

try {
Color actualRGBColor = colorConversionUtils.getRGBColor("FFFFFFA");
Color expectedRGBColor = new Color(255, 255, 255);
assertTrue("Actual color, " + actualRGBColor + ", does not equal expected
color, " + expectedRGBColor, actualRGBColor.equals(expectedRGBColor));

}
catch (IllegalArgumentException ia_excp) {
assertionFailedError = new AssertionFailedError(ia_excp.getMessage());
assertionFailedError.initCause(ia_excp);
throw assertionFailedError;
}


So far, so good. Now, here's where my problem comes in. When I execute this
test, the method in which it is situated inevitably results in a black X
decoration on the test result in the JUnit window of Eclipse. (I'm using
JUnit3 for now.) I know that's reasonable given that it is correctly
reporting an exception thrown by a method that is under test.

However, I had the distinct impression in a conversion some months ago,
that a properly written set of JUnit tests should always give me a green
checkmark decoration on every test. And that makes a lot of sense to me
too. Ideally, given that unit tests may be executed hundreds of times and
each test class may have dozens of test methods in it, the last thing
anyone wants to do is have to inspect each test method individually and
know exactly which ones should have had green checkmarks, black X's and red
X's. It's simply far easier to make sure that all of the tests had green
checkmarks and only dive into them if some test _doesn't_ have a green
checkmark.

I don't see how I can reconcile these two ideas. If I test for thrown
exceptions, I will inevitably get some black X's. (Unless there is some way
to negate the exception test so that it only gives the black X if the
exception FAILS to be thrown???) But if I get black X's, how am I to know
quickly which of the test methods SHOULD be getting black X's and which
ones should be getting green checkmarks? I don't much like the idea of
trying to document all that and I certainly can't memorize it all....

public void testParseException() {
try {
myObj.getRGB("Bad value!");
fail("Bad value did not cause exception!");
} catch(IllegalArgumentException e) {
// Success!
}
}

public void testGoodParse() {
assertEqual(white, myObj.getRGB("FFFFFF"));
}

 

[Tom Anderson]

I can easily invoke the method with a value that will cause the
exception to be thrown and I can catch it with a standard try/catch
block. I can also put code in my catch block to make sure that JUnit
reports the error from the Exception in the method being tested. I end
up with this test which correctly reports the IllegalArgumentException
within the JUnit window:

try {
Color actualRGBColor = colorConversionUtils.getRGBColor("FFFFFFA");
Color expectedRGBColor = new Color(255, 255, 255);
assertTrue("Actual color, " + actualRGBColor + ", does not equal expected
color, " + expectedRGBColor, actualRGBColor.equals(expectedRGBColor));

}
catch (IllegalArgumentException ia_excp) {
assertionFailedError = new AssertionFailedError(ia_excp.getMessage());
assertionFailedError.initCause(ia_excp);
throw assertionFailedError;
}

So far, so good.

No, not good, this is completely wrong.

You want this:

@Test(expected=IllegalArgumentException.class)
public void getRGBColorThrowsExceptionOnBadInput() {
colorConversionUtils.getRGBColor("FFFFFFA");
}

However, I had the distinct impression in a conversion some months ago,
that a properly written set of JUnit tests should always give me a green
checkmark decoration on every test.
Correct.

If I test for thrown exceptions, I will inevitably get some black X's.
(Unless there is some way to negate the exception test so that it only
gives the black X if the exception FAILS to be thrown???)

Got it in one.

tom

 

[Rhino]

public void testParseException() {
try {
myObj.getRGB("Bad value!");
fail("Bad value did not cause exception!");
} catch(IllegalArgumentException e) {
// Success!
}
}

public void testGoodParse() {
assertEqual(white, myObj.getRGB("FFFFFF"));
}

Thank you VERY much for the speedy AND accurate answer! It never occurred
to me to try that! It solves my problem very simply and elegantly.

 

[Lew]

I'm having a bit of a conceptual struggle right now with the right way to
handle exceptions in JUnit tests. Since I always explain concepts better in
terms of examples, let me describe a scenario to you which should
illustrate the situation clearly.

I have a method called getRGBColor() whose input parameter is a six letter
String that is supposed to contain exactly six hex digits. Those hex digits
are the String representation of an RGB Color. Therefore, "FFFFFF"
represents White.

I have no trouble writing a JUnit test that passes in "FFFFFF" as an input
value and then verifies that the Color returned by the method is White i.e.
Color(255,255,255) so I can test that aspect of the code without trouble.

However, the method throws IllegalArgumentException if the input String is
not precisely 6 characters long or if any of the characters are not hex
digits.

I can easily invoke the method with a value that will cause the exception
to be thrown and I can catch it with a standard try/catch block. I can also
put code in my catch block to make sure that JUnit reports the error from
the Exception in the method being tested. I end up with this test which
correctly reports the IllegalArgumentException within the JUnit window:

try {
Color actualRGBColor = colorConversionUtils.getRGBColor("FFFFFFA");
Color expectedRGBColor = new Color(255, 255, 255);
assertTrue("Actual color, " + actualRGBColor + ", does not equal expected
color, " + expectedRGBColor, actualRGBColor.equals(expectedRGBColor));

}

Your closing brace is supposed to line up at the same level as the 'try' command.

catch (IllegalArgumentException ia_excp) {

The Java naming conventions decry the use of underscores in names of
non-constant variables. Use camel case.

You've seen this suggestion before, no?

assertionFailedError = new AssertionFailedError(ia_excp.getMessage());
assertionFailedError.initCause(ia_excp);
throw assertionFailedError;

Why the heck are you throwing an error for the desired result?

} ....
I don't see how I can reconcile these two ideas. If I test for thrown
exceptions, I will inevitably get some black X's. (Unless there is some way
to negate the exception test so that it only gives the black X if the
exception FAILS to be thrown???) But if I get black X's, how am I to know
quickly which of the test methods SHOULD be getting black X's and which
ones should be getting green checkmarks? I don't much like the idea of
trying to document all that and I certainly can't memorize it all....

If the test is supposed to throw an exception, then don't throw an
AssertionFailedError in the 'catch' block. If the test is not supposed to
fall through to the comparison with the expected color, then you are not
expecting the color you assert to be expected.

You will not "get a green check" if you lie about the expected results. Tell
the truth instead; it works better.

try
{
Color actualRGBColor = colorConversionUtils.getRGBColor("FFFFFFA");
fail( "should have thrown IllegalArgumentException" );
}
catch ( IllegalArgumentException exc )
{
assertNotNull( "Null exception!?", exc );
}

 

[Tom Anderson]

Thank you VERY much for the speedy AND accurate answer! It never
occurred to me to try that! It solves my problem very simply and
elegantly.

Not as elegantly as my solution!

tom

 

[Lew]

Rhino wrote:

and failed to acknowledge Daniel Pitts, who said:

Rhino:

Not as elegantly as my solution!

Indeed. As someone who is just now getting used to JUnit 4 and who loves,
nay, adores annotations, I found tom's response exceedingly enlightening.

 

[Daniel Pitts]

Not as elegantly as my solution!

True, I'm not used to using Annotations.

Although, the pattern I used can have additional benefits. It could be
that you want to test the state of the object after the exception:

public void testFoo() {
try {
foo.doSomething("broken");
fail();
} catch (BrokenException be) {
// Success
}
assertThat(foo, hasGoodState());
}

 

[Arne Vajhøj]

I have a slight problem with using the annotation approach alone. It can
only check for a throw of appropriate type. It cannot check other
features of the thrown object, such as its message or a wrapped
exception. As far as I can tell, a catch block is the only way to fully
test if the interface promises anything beyond a throw type.

Checking the message can be necessary, but it is an indication that
some programmer were too reluctant to subclass.

Arne

 

[Tom Anderson]

I have a slight problem with using the annotation approach alone. It can
only check for a throw of appropriate type. It cannot check other
features of the thrown object, such as its message or a wrapped
exception. As far as I can tell, a catch block is the only way to fully
test if the interface promises anything beyond a throw type.

Very true. Daniel's point about not being able to make assertions about
the throwing object after the throw is also a good one. I suppose i'd
retreat slightly and say the expected exception method is appropriate when
you just want to check that the right exception is thrown, because it's
easy, it makes the intent clear, and it gives you a nice error message,
but if you want to do anything more, it's no use.

It also runs the risk of silently letting bugs pass in situations like
this:

@Test(expected=IndexOutOfBoundsException.class)
public void testGetThrowsExceptionForBadIndex() {
List<String> l = new MyList();
l.add("hello");
l.get(1);
}

This is written to check that get correctly throws an exception - but if
add incorrectly throws an exception, it will still pass.

So now i'm thinking about retreating even further.

tom

 

[Arne Vajhøj]

No, not good, this is completely wrong.

You want this:

@Test(expected=IllegalArgumentException.class)
public void getRGBColorThrowsExceptionOnBadInput() {
colorConversionUtils.getRGBColor("FFFFFFA");
}

If he can upgrade from JUnit 3 to 4.

Otherwise the fail method as show by Daniel Pitt must be the
solution.

JUnit 4 is relative old now, so I can not see any reasons not
to upgrade (at least not for a project where the work to convert
is not a problem in itself).

Arne