Code review comment for lp://staging/~fwereade/pyjuju/cobbler-connect-production

Quick responses:

[1] I guess something in my past has caused me to become fanatical about having a single point of truth for every possible piece of data in the system. What if someone changes config underneath me!? (Yeah, I'll change it.)

[2] Cheers :-).

[3] Yes it is: test_bootstrap.py, lines 43-48. I'm open to the argument that testing a single call that happens to follow this path is a little weak, but I'm reluctant to add further complexity to the already excessive mocking here. See below...

[4] Thanks, nice catch. I have a comment I'll come back to in the light of...

[5] It'll only get worse in future branches :-(. On the bright side, I think this is a good opportunity for a philosophical ramble :-p.

I'd categorise your favoured testing style as "integration" rather than "unit", which I've tended to neglect in the past, but I'm reevaluating that: factoring the generic code out of providers.ec2 was *much* easier than I expected, largely thanks to this testing style.

My own approach has historically tended to favour ludicrously tight unit testing, in which *everything* that isn't part of the SUT is mocked out; the obvious drawback is that the burden of significant refactorings can be much heavier (and it's very possible to miss tests which expect another object to have an outdated interface), but I've generally had good experiences when my unit tests are backed up by relatively few high-level functional tests: 9 times out of 10, I've found, an end-to-end test of the happy path will flush out the inconsistencies.

In the end, I think, it's a choice between testing (1) the ultimate external effects of an interaction with the SUT or (2) the precise interactions with the SUT's dependencies; which path is better in the long term depends on the interactions' relative susceptibility to change. I do feel that only (2) really qualifies as "unit" testing, but that's frankly irrelevant: reliable coverage matters a lot more than the names we happen to use for the techniques we employ.

I'm not advocating a change of approach here, but I would say that we're we're pretty close to the line: the current strategy means that a tweak to CobblerConnect's interaction with the server will lead to failures in an arbitrary number of tests, none of which are related to the cause of the failure (except coincidentally). We'll have to see how it shakes out here; I think the fact that the cobbler API is much chattier than EC2's may eventually justify a different approach locally, but it hasn't hurt me enough to seriously complain. Yet :-).

[3/4 redux] In [3], I'm comfortable testing only one interaction with the "invalid token" code, because I "know" everything goes through ._call(). In [4], there are 3 distinct operations that can hit ._check_call(), and under the current approach I can't see any option but to test both happy and sad paths each one individually... but this makes me uncomfortable, because a minor tweak to that code would entail fixing at least 6 tests. I'll fix it anyway; I just had an urge to register my misgivings ;-).

Er, I really did intend these to be quick responses. Thanks for the thorough review!