Open Stack

On 10/31/2013 03:24 PM, Jeremy Stanley wrote:
> On 2013-10-31 13:30:32 +0000 (+0000), Mark McLoughlin wrote:
> [...]
>> In cases like that, I'd be of a mind to go "+2 Awesome! Thanks for
>> catching this! It would be great to have a unit test for this, but
>> it's clear the current code is broken so I'm fine with merging the
>> fix without a test". You could say it's now the reviewers
>> responsibility to merge a test, but if that requirement then turns
>> off reviewers even reviewing such a patch, then that doesn't help
>> either.
> [...]
>
> I get the impression it was one of my +2s in a situation like this
> which spawned the thread (or at least was the straw which broke the
> camel's back), so apologies for stirring the beehive. Someone was
> trying to set up their own copy of several of our infrastructure
> projects, spotted a place in one where we had hard-coded something
> specific to our environment, and wanted to parameterize it upstream
> rather than paper over it on their end.
>
> The bug-reporter-turned-patch-contributor didn't know how to write a
> test which would confirm that parameter got passed through and we
> weren't performing tests yet for any similar parameters already in
> the daemon which I could point to as an example. I agree it's a
> judgement call between risking discouraging a new contributor vs.
> reducing test coverage further, but I was probably still a bit too
> hasty to suggest that we could add tests for those in a separate
> commit.
>
> In my case I didn't have the available time to instruct the
> contributor on how to write tests for this, but that also probably
> meant that I didn't really have time to be reviewing the change
> properly to begin with. I'm quite grateful to Robert for stepping in
> and helping walk them through it! We got more tests, I think they
> got a lot of benefit from the new experience as well, and I was
> appropriately humbled for my lax attitude over the situation which
> nearly allowed us to miss a great opportunity at educating another
> developer on the merits of test coverage.
It is probably the best approach to work with a new patch submitter to 
show them how to write the tests.  Another approach is to write a test 
yourself.  CHeck for some precondition that shows the patch is appalied, 
and if it is not met, throw a Skip exception.  That test should pass 
until their bug comes in.  Then, once htier patch comes in, test test 
should be triggered.  You can remove the skip code in a future commit.

I would like to see more of this kind of coding:  tests that show a 
feature is missing etc.  We discussed it a bit in the Keystone/Client 
discussions where we wanted to run tests against a live sever for a 
client change, but couldn't really check in new tests in the server as 
it was after code freeze.  I made the decision to push for Tempest tests 
there...and we broke some new ground in our workflow.

One tool that is really valuable is the test coverage tool.  It can show 
the lines of code that have no coverage, and will help confirm if a 
given patch is tested or not.   It used to run on each commit, or  so I 
was told.  I was running it periodically for Keystone tests: here is an 
old run http://admiyo.fedorapeople.org/openstack/keystone/coverage/