<div dir="ltr"><div class="gmail_extra"><h1 class=""><span id=":22l" class="" tabindex="-1"></span></h1>> When is it okay for submitters to say 'I don't want to add tests' ?<br><br></div><div class="gmail_extra">
When they don't want their patch merged :-) But more seriously....<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 31, 2013 at 1:07 PM, Robert Collins <span dir="ltr"><<a href="mailto:robertc@robertcollins.net" target="_blank">robertc@robertcollins.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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></blockquote><div><br></div><div>Do you mean no test suite at all for the specific project or the feature they are modifying?<br></div><div>For the latter I think its fine to require that they start one for the feature even if it just tests<br>
the behavior they just modified.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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></blockquote><div><br></div><div>Yea I have a lot of sympathy for this one. I run into this occasionally myself<br>where it can be really difficult to cleanly, yet still vaguely realistically inject<br>
an error. I'm sort of stuck on one at the moment and I've definitely spent a lot more<br>time thinking about how to write an adequate test than the fix itself.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

D - testing this won't offer benefit<br></blockquote><div><br></div><div>The test will at least always act as a check that the specific behaviour doesn't change<br>accidentally in the future won't it (in the context of bug fixes) ?<br>
</div><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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></blockquote><div><br></div><div>There is perhaps also:<br><br></div><div>H - a test in another project (say tempest) picks up this failure already.<br></div><div> <br></div><div>and H is in combination with say C. I'd have a lot of sympathy for that situation.<br>
<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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></blockquote><div><br></div><div>I agree with you on E, F and G. But in those circumstances we should<br>be offering those people help in writing tests, especially if they're new.<br></div><div><br></div>
Chris<br></div></div></div>