Tests, code quality and reviews.

[Aidan Skinner <ai...@apache.org>]

Dearest All,

We've had the more formal commit-then-review process for a few weeks
now. What we didn't really discuss was the standard to which commits
were to be held. So I'd like to take a minute, if you'll sit right
there, to tell you how we can become the prince of a town called
"adequate test coverage".

Our unit test coverage is... poor. Our system test coverage is better.
You can get a decent idea of what our unit test coverage is by
downloading cobertura (http://cobertura.sourceforge.net/) and
unpacking it into qpid/java/lib/cobertura/ then running "ant
cover-test coverage-report" and looking in build/coverage/index.html.
It'll take a while to run because of the instrumentation, particularly
SerialTest which takes an age. Running that in an module will put the
coverage report for that module in build/<module>/coverage/index.html
which is helpful.

To improve this situation, I'd like to propose that not having a test
case that covers your change is grounds for rejecting the patch. We
certainly can't get better if we continue to make the situation worse.

Thoughts?

- Aidan
-- 
Apache Qpid - World Domination through Advanced Message Queueing
http://cwiki.apache.org/qpid
"Nine-tenths of wisdom consists in being wise in time." - Theodore Roosevelt

Re: Tests, code quality and reviews.

[Alan Conway <ac...@redhat.com>]

On Wed, 2008-10-01 at 10:21 +0100, Martin Ritchie wrote:
> 2008/9/30 Rafael Schloming <ra...@redhat.com>:
> > Aidan Skinner wrote:
> >>
> >> Dearest All,
> >>
> >> We've had the more formal commit-then-review process for a few weeks
> >> now. What we didn't really discuss was the standard to which commits
> >> were to be held. So I'd like to take a minute, if you'll sit right
> >> there, to tell you how we can become the prince of a town called
> >> "adequate test coverage".
> >>
> >> Our unit test coverage is... poor. Our system test coverage is better.
> >> You can get a decent idea of what our unit test coverage is by
> >> downloading cobertura (http://cobertura.sourceforge.net/) and
> >> unpacking it into qpid/java/lib/cobertura/ then running "ant
> >> cover-test coverage-report" and looking in build/coverage/index.html.
> >> It'll take a while to run because of the instrumentation, particularly
> >> SerialTest which takes an age. Running that in an module will put the
> >> coverage report for that module in build/<module>/coverage/index.html
> >> which is helpful.
> >>
> >> To improve this situation, I'd like to propose that not having a test
> >> case that covers your change is grounds for rejecting the patch. We
> >> certainly can't get better if we continue to make the situation worse.
> >>
> >> Thoughts?
> >
> > +1
> >
> > --Rafael
> 
> +1 from me to. We can't expect to have better code if we don't at
> least try and test what we are writing.
> 
> 

+1 - only case where a test is not absolutely needed is a pure refactor,
and even then you can always improve the existing tests :)

Re: Tests, code quality and reviews.

[Martin Ritchie <ri...@apache.org>]

2008/9/30 Rafael Schloming <ra...@redhat.com>:
> Aidan Skinner wrote:
>>
>> Dearest All,
>>
>> We've had the more formal commit-then-review process for a few weeks
>> now. What we didn't really discuss was the standard to which commits
>> were to be held. So I'd like to take a minute, if you'll sit right
>> there, to tell you how we can become the prince of a town called
>> "adequate test coverage".
>>
>> Our unit test coverage is... poor. Our system test coverage is better.
>> You can get a decent idea of what our unit test coverage is by
>> downloading cobertura (http://cobertura.sourceforge.net/) and
>> unpacking it into qpid/java/lib/cobertura/ then running "ant
>> cover-test coverage-report" and looking in build/coverage/index.html.
>> It'll take a while to run because of the instrumentation, particularly
>> SerialTest which takes an age. Running that in an module will put the
>> coverage report for that module in build/<module>/coverage/index.html
>> which is helpful.
>>
>> To improve this situation, I'd like to propose that not having a test
>> case that covers your change is grounds for rejecting the patch. We
>> certainly can't get better if we continue to make the situation worse.
>>
>> Thoughts?
>
> +1
>
> --Rafael

+1 from me to. We can't expect to have better code if we don't at
least try and test what we are writing.


-- 
Martin Ritchie

void FieldTable::erase(const std::string& name)

[Carl Trieloff <cc...@redhat.com>]

About to bring this function back from the dead, any specific reason it 
was removed?

Carl.

Re: Tests, code quality and reviews.

[Rafael Schloming <ra...@redhat.com>]

Aidan Skinner wrote:
> Dearest All,
> 
> We've had the more formal commit-then-review process for a few weeks
> now. What we didn't really discuss was the standard to which commits
> were to be held. So I'd like to take a minute, if you'll sit right
> there, to tell you how we can become the prince of a town called
> "adequate test coverage".
> 
> Our unit test coverage is... poor. Our system test coverage is better.
> You can get a decent idea of what our unit test coverage is by
> downloading cobertura (http://cobertura.sourceforge.net/) and
> unpacking it into qpid/java/lib/cobertura/ then running "ant
> cover-test coverage-report" and looking in build/coverage/index.html.
> It'll take a while to run because of the instrumentation, particularly
> SerialTest which takes an age. Running that in an module will put the
> coverage report for that module in build/<module>/coverage/index.html
> which is helpful.
> 
> To improve this situation, I'd like to propose that not having a test
> case that covers your change is grounds for rejecting the patch. We
> certainly can't get better if we continue to make the situation worse.
> 
> Thoughts?

+1

--Rafael