Open Stack

On 2013-10-31 09:05, Kyle Mestery (kmestery) wrote:
> 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.

When in doubt, I tend to err on the side of "no score with comments".  
It's not ideal for every situation, but I like to think it gets my point 
across without making it sound like I disapprove of the change itself.  
If my suggestions are ignored completely, I've always got the -1 in my 
back pocket to press the issue. :-)

-Ben