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.
