I like Steve's suggestion:
> 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.
I'm (almost!) a first time contributor and I've put a fix on the backburner
as I find the time to ramp up on the unit tests suite. The fix was
"review"ed 2 weeks ago, requesting unit tests - very reasonable but I
needed help to even start (I got no response from requests on IRC, ML, or
e-mail to the bug reporter).
If it wasn't for the good Upstream University people mentoring me - heh,
they deserve a plug! - I'm sure I would have moved on.
Thankfully, a cunning commit message provoked the guidance I needed so I
just need a long weekend to get back to the tests - which I have now.
I'm not sure in which cases you would/wouldn't want to split the bug for
the implementation of tests but I'm sure it would help other first timers
and encourage new contributors.
Regards,
Mike
On 31 October 2013 19:51, Steven Hardy <sha...@redhat.com> wrote:
> 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
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev@lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
