Monday, July 19, 2004

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...

6 Comments:

Blogger Randall said...

I think you may have missed the point to doing code-coverage tracking. I think there's a lot of software out there with segments of code that has not been executed by the developer before going out the door. There is no excuse...except the normal litany of excuses. If we can find any way to cut this out then we've taken huge steps forward.

July 20, 2004 3:19 PM  
Blogger Chris said...

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...

July 20, 2004 7:07 PM  
Anonymous Anonymous said...

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?

July 21, 2004 2:46 AM  
Blogger Chris said...

Doh...

Correcting the mistakes now ... Thanks for the heads up...

July 21, 2004 8:04 AM  
Blogger John said...

This has little do with code coverage and everything to do with a lack of a documented contract.

Code coverage tells you nothing regarding semantic correctness. Your routine was fine (ignoring its string operands) right from the start.

Returning an arbitrary string to indicate some 'out of band' or 'operational failure' is obstification at its worse and introduces a totally a non-standard condition set for clients to check against (we're back to 1970s C return codes).

The correct response for the test case of 'bob' + 'bill' is an exception. If its anything else then it should be documented appropriately and even then would have a very hard time justifying itself as youre reinventing the wheel and making it harder for other to use (it would invariably have to be wrapped to reinterpret your custom 'exception conditions' into language standard representations to be open and standard).

If you take the stance that the test case is the documented contract then your routine simply lacked its proper functionality to start with and was incomplete for production use. The fact you had 100% code coverage tells you nothing about whether is works properly, but thats a obvious:-

public String getQuoteOfTheDay( const Date )
{
return( "Not done yet" );
}

Cluttering code with unnecessary and nonsensical state checks and communicating to clients in non-standard ways is not good practice.

(Note, in exposed memory model languages this kind of checking might be deemed desirable if one wants run-time diagnostics rather than static error tracing).

The correct thing to say about code coverage is that: -

100% code coverage makes no guarantees regarding semantic correctness.

and that

semantic correctness can only be asserted with 100% code coverage. Otherwise you haven't tested all the execution paths and have therefore missed a test case or should re-factor the redundant code.

December 18, 2004 11:16 AM  
Blogger Daniel Marchant said...

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.

January 04, 2005 5:08 PM  

Post a Comment

<< Home