100% code coverage no guarentee...
Note: I have edited this entry on July 19, 2004 to clean-up the code after someone pointed out some of the errors below.
A bit of reading lately about code coverage, and aiming for 100% and whether that is realistic or not...
It's important for people to step back and not just a code coverage, but the quality of the unit test cases that they are writing...
We'll look at a function to take 2 strings, convert the strings to numbers, add them together and return the result as a string (yes - we all know that when coverting strings to numbers to add them together, you should take a Vector... just kidding... for demonstration purposes only folks...)
Ex:
/**
* Add 2 numbers two numbers together...
*/
public String addNumbers(String value1, String value2) {
int i1 = Integer.parseInt(value1);
int i2 = Integer.parseInt(value2);
return("" + (i1+i2));
}
We could write a testcase:
public void testAddNumbers() {
int i1 = 5;
int i2 = 10;
assertTrue((""+(i1+i2)).equals(addNumbers(""+i1,""+i2)));
}
Bingo ... I've tested that my function works, and I've got 100% code coverage...
But what happens when I take user input and pass it directly into my function ... And the user doesn't type a number... Let's simulate in our testcase...
public void testAddNumbers() {
int i1 = 5;
int i2 = 10;
assertTrue((""+(i1+i2)).equals(addNumbers(""+i1,""+i2)));
assertTrue(("NaN".equals(addNumbers("bob","doug")));
}
Hmmm... My function is throws a NumberFormat exception... and my test case is clearly failing... We'll update our function:
/**
* Add 2 numbers two numbers together...
* @return sum of two numers, or "NaN" if either string is not a number...
*/
public String addNumbers(String value1, String value2) {
try {
int i1 = Integer.parseInt(value1);
int i2 = Integer.parseInt(value2);
return("" + (i1+i2));
} catch (NumberFormatException nfe) {
// debug: nfe.printStackTrace();
return("NaN");
}
}
Now my testcase is passing, I'm back at 100% code coverage, and all is wonderful... Until the guy in the next cubicle start's passing null's into my function...
We update the testcase:
public void testAddNumbers() {
int i1 = 5;
int i2 = 10;
assertTrue((""+(i1+i2)).equals(addNumbers(""+i1,""+i2)));
assertTrue("NaN".equals(addNumbers("bob","doug")));
assertTrue("NaN".equals(addNumbers("bob",null)));
}
Now I'm getting NullPointerExceptions...
Update the function:
/**
* Add 2 numbers two numbers together...
* @return sum of two numers, or "NaN" if either string is not a number...
*/
public String addNumbers(String value1, String value2) {
try {
int i1 = Integer.parseInt(value1);
int i2 = Integer.parseInt(value2);
return("" + (i1+i2));
} catch (NumberFormatException nfe) {
// debug: nfe.printStackTrace();
return("NaN");
} catch (NullPointerException npe) {
// debug: npe.printStackTrace();
return("NaN");
}
}
... You get the point... This can go on for quite a while ... Someone might pass in "4.5" and "3" and expect "7.5" - but won't get it because our implementation is using Integers... We've had 100% code coverage all along, but found that some input throws Runtime exceptions, that if uncaught, would bring our program down entirely...
While code coverage is a great and noble goal we should all strive for, it is no guarentee that the code won't fail...
4 Comments:
I'm not saying code coverage isn't a good thing... I am saying that just because you have 100% code coverage does not mean that your code is correct, or free of bugs... All of my examples had 100% code coverage, but were all capable of failing to various degrees - from throwing runtime exceptions to not evaluating 3 + 4.5 correctly... People who focus on code coverage are ensuring that all branches are executed, but aren't necessarily ensuring that the results are correct and/or are not ensuring that runtime exceptions don't crop up and kill their software...
Erm... addNumbers() takes Strings yet you seem to be passing ints to it. Also, you assign to i1 and i2 in the test method without declaring them (they are declared in addNumbers() but only as locals). Did you actually run this?
Doh...
Correcting the mistakes now ... Thanks for the heads up...
You could have just had:
if((value1 == null) || (value2 == null) ) {
return ("NaN");
}
at the beginning of the line, but that is beside the point.
I think the point is a good one that coverage doesn't reflect the quality necessarily. Also coverage is skewed as well if people are running unit tests without having calls for the simple get / set beans.
I do think the edge cases should be tested but the assertion could be asserting they failed properly adding to the contract of the component.
Post a Comment
<< Home