Code review comment for lp:~jtv/maas-test/ssh-key

Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

> - I'm really not fond of find_matching_call() and the refactoring that goes
> with. I think you're relaxing the assertions done in the tests too much with
> this. In my opinion, it's better to keep checking the complete commands that
> are run. It's a sort of double-entry bookkeeping system that is there to
> guarantee that any change to the code is voluntary and I don't think the tests
> are brittle at all.

I see that there's some missing test coverage on setUp as a whole, and I'll fix that. But there is no refactoring here that went with find_matching_call — it's entirely the other way around. I first ran into the problem of test brittleness. Several tests really were going to break because of unrelated changes to command lines. The choice was one of increasing it or decreasing it, and I first specified find_matching_call to avoid increasing it. I only wrote the actual function once I saw that it allowed me to solve that problem. Adding another option to the ssh command line will now break one test, but no more.

Tests are like double-entry bookkeeping, but double-entry bookkeeping is not writing down the same transaction in multiple places like these tests were doing. Rather, it's a system of recording complementary aspects of a transaction. Failure of the two records to match up is a crucial signal that something went wrong. False positives, and the habit of updating the tests to match the code, degrade the value of that signal. That's how double-entry bookkeeping devolves towards forms-in-triplicate: a bureaucratic exercise that slows things down and confuses, and doesn't improve quality much. It's also self-reinforcing, which is why it's so important to fight the tendency.

« Back to merge proposal