Open Stack

On 10/30/2013 11:37 PM, 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
> 
> 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.
> 
> Now, if I'm wrong, and folk have different norms about when to accept
> 'reason X not to write tests' as a response from the submitter -
> please let me know!

I've done a lot of thinking around this topic [1][2] and really it comes
down to this: everything can be tested and should be.

There is an argument to A, but that goes beyond the scope of our use
case I think. If I hear B, I would suspect the tests aren't unit tests,
but are functional/integration tests (a common problem in OpenStack).
Functional tests are brittle and usually have painful setup sequences.
The other cases fall into the -1 camp for me. Tests required.

That said, recently I was -1'ed for not updating a test, because I added
code that didn't change the program flow, but introduced a new call.
According to my rules, that didn't need a test, but I agreed with the
logic that people would be upset if the call wasn't made (it was a
notification). So a test was added. Totally valid argument.

TL;DR: Tests are always required. We need to fix our tests to be proper
unit tests and not functional/integration tests so it's easy to add new
ones.

-S


[1]
http://www.sandywalsh.com/2011/06/effective-units-tests-and-integration.html
[2]
http://www.sandywalsh.com/2011/08/pain-of-unit-tests-and-dynamically.html


> -Rob
>