Devel::Cover, Testing, 100%?!

[BUU]

I'm having fun writing a test suite for my latest module. I just have one problem, I can't get to 100% coverage! My problem is basically thus: I have a function that takes input and does two operations.

The first operation iterates over the data and munges it in to the proper format, the second operation iterates over the correct format and does $stuff. My problem is, in the second operation I have a test for "bad data" while I'm iterating.

As far as I can tell, theres no possible way for this to happen, as the first step weeds out all the bad data and only passes "good" data to the second step. By "no possible way" I mean theres nothing I can think of to feed the function that will result in that second operation error message. As a result, that branch is never tested/called and I don't get to 100% coverage.

So my question is, do I remove that extra test so I get to 100% or just leave it in and live with 98.5%?

[dragonchild]

If you can mathematically show that the second validation step will never be called, then you have a redundant check. This is one of the odder things test coverage will show.

I'd remove it and let CVS keep the memory. You're not removing it to get to 100% test coverage ... you're removing it because it's a useless piece of code.

[adrianh]

The first operation iterates over the data and munges it in to the proper format, the second operation iterates over the correct format and does $stuff. My problem is, in the second operation I have a test for "bad data" while I'm iterating.

If it were me I would remove the second test. Why check for data that you have already checked is valid?

[Crian]

I would delete the obsolete second test.

If you don't want to remove it, then comment out the first check (in the first programm), so you get garbage into your second program and your error-case will be testet.

Or just put a "1 or" in front of your if-expression to test the output from your error-message.

But you cannot create a testsuit covering all cases, if you have redundant testcode.

[hv]

I'm a belt-n-braces coder, and I believe it is perfectly reasonable to have a test in one part of the code that will never be triggered unless at some point in the future another part of the code changes. But it is notoriously difficult to test that all possible error paths are correctly handled.

Consider whether it might make sense to split the routine into two - some methodologies would in any case consider it bad form to have a function that performs two operations. This would allow you separately to test that the first routine cleans the data, and that the second routine correctly barfs on bad data.

Hugo

[tachyon]

Just a note. While 100% coverage is a fine goal you may still have issues. Having a test does not make it a good test. My classic example (really happened) is:

sub is_integer { 
    return 0 unless defined $_[0];
    return $_[0] =~ m/^\d$/ ? 1 : 0 
}

The guy that wrote this snippet of code had 10 tests for it! He tested the numbers 0..9 and it passed every test...... It broke on demo with client 10 whose ID was not a valid integer according to that code.....

So the moral is that coverage is one aspect but testing the data range, edge cases, and totally invalid data is another.

[stvn]

So my question is, do I remove that extra test so I get to 100% or just leave it in and live with 98.5%?

To start with, you don't need to get 100%. Anything is better than nothing, and anything over 90% should be acceptable as long as you know where you are loosing that other couple of %s. There has been some discussion on the perl-qa list about this in the last month of so, and even Paul Johnson (the author of Devel::Cover) has said, that 100% is alot of times an unreasonable/unreachable goal. Branch and Conditional coverage are notoriously difficult to get 100% on, and things like

my $class = ref($_class) || $_class;

which are very common perl idioms, tend to not get tested (and really dont need to be). Unless your pointy haired boss is breathing down your neck to get 100%, I would be happy with 98.5%. And if your boss really insists, then write the silly/extra/redundant tests to get that 100%, as its no more silly than insisting on that 100%.

As far as I can tell, theres no possible way for this to happen....

Sometimes, this is what Devel::Cover is best at. Showing you code that you dont actually need. If you can't reach the code to test it, then you likely should delete it as its just wasting opcodes.

However, if you don't trust yourself (or others who might touch the code) to not break "step 1" at some point, in a way that "step 2" will fail due to bad input, then leave it in and live with your 98.5%.

This might also be a perfect refactoring opportunity, maybe step 1 and step 2 should/could be different subroutines? Not only would this allow you to test them easily, but it would allow you at a later date to vary the step 1 process to handle different input (to be converted to step2 acceptable output). Just a thought.

[TomDLux]

You might consider dividing the functionality into two routines: weeder() and processor(), which you can test separately. Then have a third routine which is the one that is the API, which you can test separately. it conssist of:

sub API {
   processor(weeder( @_ ));
}