Open Stack

On Thu, Oct 31, 2013 at 01:30:32PM +0000, Mark McLoughlin wrote:
> 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.

The approach the Heat team have sometimes taken in this situation is to
merge the patch, but raise a bug (targetted at the next milestone)
identifying the missing coverage.

This has a few benefits (provided the patch in question is sufficiently
simple that patches aren't deemed essential)
- The bug gets fixed quickly
- The coverage still gets added, and we keep track of the gaps identified
- Less chance of discouraging new submitters
- Provides some "low hanging fruit" bugs, which new contributors can pick
  up

I'm not saying we do this routinely, but it's a possible middle ground to
consider between "-1 fix all our tests" and "+2 shipit!"

I think something that's important to recognise is that sometimes (often?)
writing tests is *hard*.  I mean, look at the barriers to entry, you need
to have some idea about tox, nose, testr, mox, mock, testscenarios, etc etc

The learning curve is really steep, and thats before you start considering
project specific test abstractions/fixtures/mocking-patterns which can all
be complex and non-obvious.

So +1 on a bit of compassion when reviewing, particularly if the
contributor is a new or casual contributor to the project.

Steve