Open Stack

On Oct 31, 2013, at 8:56 AM, Clint Byrum <clint at fewbar.com> wrote:
> Excerpts from Mark McLoughlin's message of 2013-10-31 06:30:32 -0700:
>> 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.
>> 
> 
> Even with a long time contributor, empathy is an important part of
> constructing reviews. We could make more robotic things that review for
> test coverage, but we haven't, because this is a gray area. The role of
> a reviewer isn't just get patches merged and stop defects. It is also to
> grow the other developers.
> 
>> 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 ...
>> 
> 
> This goes back to my recent suggestion to help the person not with a -1
> or a +2, but with an additional patch that fixes it.
> 
>> 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.
>> 
> 
> I understand entirely why you choose this, and I think that is fine. I,
> however, see this as a massive opportunity to teach. That code was only
> broken because it was allowed it to be merged without tests. By letting
> that situation continue, we only fix it today. The next major refactoring
> has a high chance now of breaking that part of the code again.
> 
> So, rather than +2, I suggest -1 with compassion. Engage with the
> submitter. If you don't know them, take a look at how hard it would be
> to write a test for the behavior and give pointers to the exact test
> suite that would need to be changed, or suggest a new test suite and
> point at a good example to copy.
> 
>> 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.
>> 
> 
> I'm not going to claim that I've always practiced "-1 with compassion",
> so thanks for reminding us all that we're not just reviewing code, we
> are having a dialog with real live people.
> 
I think this is the key thing here, thanks for bringing this up Clint. At the
end of the day, patches are submitted by real people. If we want to grow
the committer base and help people to become better reviewers, taking
the time to show them the ropes is part of the game. I'd argue that is in
fact part of what being a core is about in fact.

Thanks,
Kyle

> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev