Open Stack

On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote:
> This is a bit of a social norms thread....
> 
> I've been consistently asking for tests in reviews for a while now,
> and I get the occasional push-back. I think this falls into a few
> broad camps:
> 
> A - there is no test suite at all, adding one in unreasonable
> B - this thing cannot be tested in this context (e.g. functional tests
> are defined in a different tree)
> C - this particular thing is very hard to test
> D - testing this won't offer benefit
> E - other things like this in the project don't have tests
> F - submitter doesn't know how to write tests
> G - submitter doesn't have time to write tests

Nice breakdown.

> Now, of these, I think it's fine not add tests in cases A, B, C in
> combination with D, and D.
> 
> I don't think E, F or G are sufficient reasons to merge something
> without tests, when reviewers are asking for them. G in the special
> case that the project really wants the patch landed - but then I'd
> expect reviewers to not ask for tests or to volunteer that they might
> be optional.

I totally agree with the sentiment but, especially when it's a newcomer
to the project, I try to put myself in the shoes of the patch submitter
and double-check whether what we're asking is reasonable.

For example, if someone shows up to Nova with their first OpenStack
contribution, it fixes something which is unquestionably a bug - think
typo like "raise NotFund('foo')" - and testing this code patch requires
more than adding a simple new scenario to existing tests ...

That, for me, is an example of "-1, we need a test! untested code is
broken!" is really shooting the messenger, not valuing the newcomers
contribution and risks turning that person off the project forever.
Reviewers being overly aggressive about this where the project doesn't
have full test coverage to begin with really makes us seem unwelcoming.

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.

So, with all of this, let's make sure we don't forget to first
appreciate the effort that went into submitting the patch that lacks
tests.

Mark.