Open Stack

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