[openstack-dev] When is it okay for submitters to say 'I don't want to add tests' ?
vishvananda at gmail.com
Tue Nov 12 00:42:03 UTC 2013
On Oct 31, 2013, at 6: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.
I prefer Mark's approach here. I have seen him comment that the code could
use a test, and submitted a dependent patch with the test implemented. We have
a very long average time from 1st patch to the gate in nova especially, and
going back and forth on simple bug fixes doesn't just cost the initial submitter
time, it increases the backlog and adds extra time for other reviewers as well.
It also gives the submitter a specific example of a well-written test, which
can be a faster way to learn than forcing them to get there via trial and error.
>> 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
> 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.
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
More information about the OpenStack-dev