<div dir="ltr"><div><br></div>I like Steve's suggestion:<div><br><div>> <span style="font-family:arial,sans-serif;font-size:12.727272033691406px">The approach the Heat team have sometimes taken in this situation is to</span></div>
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">> merge the patch, but raise a bug (targetted at the next milestone)</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px">
<span style="font-family:arial,sans-serif;font-size:12.727272033691406px">> identifying the missing coverage.</span><br style="font-family:arial,sans-serif;font-size:12.727272033691406px"><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">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).</span></div>
<div><br></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">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.</span></div>
<div><br></div><div><font face="arial, sans-serif">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.</font></div><div><font face="arial, sans-serif"><br>
</font></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">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.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px">Regards,</span></div><div><font face="arial, sans-serif">Mike</font></div>
<div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:12.727272033691406px"><br>
</span></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On 31 October 2013 19:51, Steven Hardy <span dir="ltr"><<a href="mailto:shardy@redhat.com" target="_blank">shardy@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Oct 31, 2013 at 01:30:32PM +0000, Mark McLoughlin wrote:<br>
> On Thu, 2013-10-31 at 15:37 +1300, Robert Collins wrote:<br>
> > This is a bit of a social norms thread....<br>
> ><br>
> > I've been consistently asking for tests in reviews for a while now,<br>
> > and I get the occasional push-back. I think this falls into a few<br>
> > broad camps:<br>
> ><br>
> > A - there is no test suite at all, adding one in unreasonable<br>
> > B - this thing cannot be tested in this context (e.g. functional tests<br>
> > are defined in a different tree)<br>
> > C - this particular thing is very hard to test<br>
> > D - testing this won't offer benefit<br>
> > E - other things like this in the project don't have tests<br>
> > F - submitter doesn't know how to write tests<br>
> > G - submitter doesn't have time to write tests<br>
><br>
> Nice breakdown.<br>
><br>
> > Now, of these, I think it's fine not add tests in cases A, B, C in<br>
> > combination with D, and D.<br>
> ><br>
> > I don't think E, F or G are sufficient reasons to merge something<br>
> > without tests, when reviewers are asking for them. G in the special<br>
> > case that the project really wants the patch landed - but then I'd<br>
> > expect reviewers to not ask for tests or to volunteer that they might<br>
> > be optional.<br>
><br>
> I totally agree with the sentiment but, especially when it's a newcomer<br>
> to the project, I try to put myself in the shoes of the patch submitter<br>
> and double-check whether what we're asking is reasonable.<br>
><br>
> For example, if someone shows up to Nova with their first OpenStack<br>
> contribution, it fixes something which is unquestionably a bug - think<br>
> typo like "raise NotFund('foo')" - and testing this code patch requires<br>
> more than adding a simple new scenario to existing tests ...<br>
><br>
> That, for me, is an example of "-1, we need a test! untested code is<br>
> broken!" is really shooting the messenger, not valuing the newcomers<br>
> contribution and risks turning that person off the project forever.<br>
> Reviewers being overly aggressive about this where the project doesn't<br>
> have full test coverage to begin with really makes us seem unwelcoming.<br>
<br>
</div></div>The approach the Heat team have sometimes taken in this situation is to<br>
merge the patch, but raise a bug (targetted at the next milestone)<br>
identifying the missing coverage.<br>
<br>
This has a few benefits (provided the patch in question is sufficiently<br>
simple that patches aren't deemed essential)<br>
- The bug gets fixed quickly<br>
- The coverage still gets added, and we keep track of the gaps identified<br>
- Less chance of discouraging new submitters<br>
- Provides some "low hanging fruit" bugs, which new contributors can pick<br>
up<br>
<br>
I'm not saying we do this routinely, but it's a possible middle ground to<br>
consider between "-1 fix all our tests" and "+2 shipit!"<br>
<br>
I think something that's important to recognise is that sometimes (often?)<br>
writing tests is *hard*. I mean, look at the barriers to entry, you need<br>
to have some idea about tox, nose, testr, mox, mock, testscenarios, etc etc<br>
<br>
The learning curve is really steep, and thats before you start considering<br>
project specific test abstractions/fixtures/mocking-patterns which can all<br>
be complex and non-obvious.<br>
<br>
So +1 on a bit of compassion when reviewing, particularly if the<br>
contributor is a new or casual contributor to the project.<br>
<br>
Steve<br>
<div class="HOEnZb"><div class="h5"><br>
_______________________________________________<br>
OpenStack-dev mailing list<br>
<a href="mailto:OpenStack-dev@lists.openstack.org">OpenStack-dev@lists.openstack.org</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</div></div></blockquote></div><br></div>