Refactor and test deploy

[larsbutler]

This adds a lot of missing test coverage for the zpm deploy code path.

The most significant change is the addition of the Deploy class, which encapsulates most of the functionality.

[larsbutler]

Okay, revisions are done. This should be ready to go.

This patch should increase test coverage from about 57% to about 68%.

[mgeisler]

Creating a single-use class that is instantiated just to be called looks bad to me. It indicates to me that this should have been a function instead of a class.

I'm also concerned about replacing 41 simple lines with 126 new lines. Instead of one mostly linear function, we now have a class with six methods, all of them private except for the constructor and the __call__ method. That does not look like a net improvement to me. Restructuring the code to support testing can make sense when it improves the overall flow, but here it is overkill and turns the code into a maze of small functions.

When discussing this with Lars, the argument he gives is that this is the price we need to pay to be able to test the deploy function. I'm uneasy with adding 201 lines of code to test the functionality of the old 41 lines. The new unit tests are strongly coupled with the code and in my opinion, this leads to fragile tests. The problem is that tests like this will need updating for almost all changes to the new code.

In other words: there is a non-negative cost associated with fine-grained unit tests. I believe this is something we should not disregard easily.

[larsbutler]

The problem I am attempting to solve with this pull request is to improve test coverage in this area of the code (so that future changes can be made with some confidence that there won't be regressions). I have regrouped the code in this way because I found it easier to test.

[larsbutler]

Deployer is no longer a callable. Instead, you just call a deploy method.

[willkelly]

I would be more inclined to make deploy a function, but the current code does not jump out at me as being bad style -- this type of approach seems pretty common in python code, and not difficult to read -- the methods are grouped linearly within the file, and when you find one, you find them all. It takes very little time to read the entirety of the class, and while there are more parts to it, it seems likely that whichever part that the reader cares about will be visible immediately.

We do end up with greater coverage. While the tests enforce a hard contract on internal function calls like prepare_auth and may introduce overhead, it seems likely that the only time prepare_auth is going to be modified is for new auth strategies, at which point we would add a new test in this method.

There is an open question here as to whether test coverage is more important than terseness and ease of code modification.

At this point in the zpm lifecycle (small user base, likely to change radically as we determine good workflows for building zapps), I believe that shooting for awesome test coverage may be premature. Nonetheless, it seems a little silly to me to reject code for being too well tested :)

+1 on this being merged as is. If it turns out that the tests or Deployer class are a major source of frustration in the future, mg can have an I told you so, and we can change back.

[mgeisler]

@willkelly The line that made me object was this

Deploy(conn, args.target, args.zapp, auth_opts)()

(I believe that's how the code looked originally.) That made it very obvious to me that a class isn't needed here — an instance is clearly also not needed since it's throw away right away.

Lars has now split into two lines to make it look like an instance is needed:

deployer = Deployer(conn, args.target, args.zapp, auth_opts, execute=args.execute)
deployer.deploy()

That's marginally nicer, but it still turns what should be an action (function!) into an opaque and stateful thing (a class).

Nonetheless, it seems a little silly to me to reject code for being too well tested :)

I like to have the code tested in general. I'm just saying that there is a cost to tests. Too-detailed tests obstruct the code in the future since you need to maintain them every time you change something. The same goes for large docstrings: they might look like a goal in themselves, but they are also a maintenance burden.

[mgeisler]

The pre-auth you talk about was removed in 296cab4acd93caf733ef1de1b6c6512f47a8ee4d when we introduced the new Swift client.

[willkelly]

mg: to be clear, what would you suggest Lars do? [DISCLAIMER: the following sentence reflects my opinion of someone else's opinion and is not a direct representation of someone else's opinion] It sounds like you are in favor of dropping the pull request in its entirety as you do not desire the tests at all. Is that the case?

[mgeisler]

@willkelly I would have preferred to see more high-level tests. Now that the tests are here, we might as well merge them. What I would like Lars to do is to remove the unnecessary class. That's all now.

[larsbutler]

@mg I don't agree with the statement that this is an "unnecessary class". On the contrary, I find it to be a rather logical grouping for the various deployment steps, and furthermore I find it to be much more reasonable to test all of the code paths this way. (If you'd like, you could submit your own pull request to improve the test coverage. Patches are welcome.)

The original reason why I wanted to add tests to this code is because I intended to add functionality to it. (See #128.) I don't want to be in a situation where I have extend untested code, so I did this first. If you add features to an untested piece of code, you're just asking for regressions.

In general, I prefer functions and procedures over an object-oriented approach. However, in my initial attempt at adding tests for this code, creating this class seemed to make the most sense. Having a monolithic deploy function makes for monolithic tests, which is what I want to avoid.