hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Christopher Lenz <cml...@gmx.de>
Subject Re: Test Coverage
Date Fri, 20 Jun 2003 13:00:50 GMT
Adrian Sutton wrote:
> Howdy all,
> We now have a license for clover to analyze our test cases and am now 
> just starting to work through adding test cases to improve our code 
> coverage.  I've very quickly come to the realization that 100% code 
> coverage may actually be a bad thing.  I've gotten AuthChallengeParser 
> to 100% coverage now so let me use it as an example:
> There are four test cases that I consider pedantic and 1 of those that I 
> really don't like.  The pedantic ones are:
> * test that AuthChallengeParser.extractScheme(null) throws an illegal 
> argument exception instead of a null pointer exception.  What difference 
> does this really make?  The only reason I can think of to keep the test 
> is so that the implied interface for the method is kept the same (it 
> never throws a NPE but throws an illegal argument exception instead).
> * Test that AuthChallengeParser.extractParams(null) throws an illegal 
> argument exception not a NPE.  Same as above really.
> * testParamsWithNoName().  Check that 
> AuthChallengeParser.extractParams("Basic realm=\"test\",=\"invalid\""); 
> throws a MalformedChallengeException because there is no name part to 
> the name/value pair.  This is good if we're in strict mode but if not 
> wouldn't it be better to be lenient and just ignore that param (with a 
> logged warning)?

I wouldn't call these tests "pedantic". They verify the contract of the 
class. The contract is either defined explicitly by being documented, or 
implicitly defined by the current behaviour, which someone may depend on 
(some other class inside HttpClient, or an external API client).

> The one I really don't like is:
> * Test that AuthChallengeParser.extractScheme(" Basic realm=\"realm\""); 
> throws a MalformedChallengeException because there's a leading space.  
> Now in strict mode that's fine and within HttpClient the leading space 
> is stripped before being passed in but it seems overly strict to me.

Again, if the contract of that method says that a leading white space -- or 
some other irregularity -- causes a specific exception to be thrown, it is 
good that there are test cases to verify the contract.

Maybe it is not the test that you dislike, but rather the behaviour of the 
method? In that case the contract should be changed to say "any leading or 
trailing white space will be trimmed" or something like that. But that has 
nothing to do with the test being invalid IMHO.

> Now, I don't mind what happens with any of these decisions to be honest 
> as none affect the actual behaviour of HttpClient - they are very much 
> edge cases.  I would however like to set up a policy on the types of 
> test cases I should create (do we want to avoid testing things like the 
> pedantic things above) as well as the best way to keep track of 
> questionable or overly pedantic test cases.  Currently I'm just adding a 
> // pedantic  above any test case that seems pedantic and a todo comment 
> over anything that I think may require a change to the code but isn't 
> clearly a bug.
> I figure from time to time I can provide a list of issues that need to 
> be considered as I work my way through the codebase.
> Personally, I'm hoping to achieve 100% test coverage firstly because 
> I've discovered how dependent I am on having good test cases while 
> working on HttpClient (most people don't have the detailed level of 
> knowledge that Mike and Oleg do and thus aren't aware that a change will 
> break some other section of code - NTLM is a regular victim of this).  
> Also, aiming for 100% coverage makes a very clear-cut decision on when 
> the job is done which make life easier as well and makes it much more 
> noticeable when new test cases need to be added.
> Any thoughts?

Achieving 100% (or any other hard number) of code coverage is a lot of work, 
and almost never guarantees that the code is free of errors. Making some 
percentage of code coverage a hard requirement is usually missing the point. 
Of course, more coverage is usually good, but there's a point of diminishing 

Anyway, while tests like the "pedantic" ones you outlined are probably not 
critical, I see no reason to throw them away if they're already present. 
They serve the very good purpose of testing error conditions and asserting 
the contract of the class under test.

Just my two cents.


View raw message