I can't test like I'm supposed to
So, I'm toward the end of converting my certificate handling in Puppet to using my nifty new indirection features, which will handle both reading and writing certs to disk and to remote servers (for signing). Yee-haw. The only bit left (I hope) is the certificate authority itself.
In the course of this work, I've created a 'SSL::Host' class that is basically a composite of a key, a certificate request, and a certificate, so it makes sense to treat a CA as a special case of that: It's various files are stored in different places, and it can sign certificates, but it's otherwise the same.
So, I need this class to read and write to different locations (easy, although not yet tested). I also want the CA to initialize itself completely upon creation -- that is, when I call CertificateAuthority.new, I want it to create and write to disk all of the files it needs. I think this is reasonable, because I don't think it's reasonable to either require the caller to call a setup method of some kind, nor is it reasonable to do it late-binding, since initialization failures would show up during a call to sign which is stupid.
Okay. Seems easy. Here's the kicker: I want to test that the CA always chooses its name as the value of the certname Puppet setting. It's pretty easy to write that basic test:
it "should always set its name to the value of :certname" do
Puppet.settings.expects(:value).with(:certname).returns("whatever")
Puppet::SSL::CertificateAuthority.new.name.should == "whatever"
end
Except... all that initialization stuff is happening (by design). So now I'm in the position of using this kind of hack:
Puppet::SSL::CertificateAuthority.any_instance.stubs(:setup_ca)
Which is mostly a hack just because that method is private -- you never need to call it, yet here we need to know it exists just for testing.
The only other option is literally about 15 lines of stubs, since the CA uses a bunch of other settings, which now all need to be stubbed because of my initial expectation, plus needing to stub out any file reading or writing.
I keep thinking that I'm just crazy, at some point I'll see the light and be able to write single-expectation tests with no setup code like Jay Fields recommends, but in reality, I think Jay is crazy. I can't fight that feeling, and I less and less want to.
Wed, 12 Mar 2008 | Tags: ruby, development, testing, mocks
Luke,
When you say:
"I also want the CA to initialize itself completely upon creation -- that is, when I call CertificateAuthority.new, I want it to create and write to disk all of the files it needs."
You are describing behavior. This is a required behavior for the class.
That means that you should have a spec for that behavior. Even if the behavior is implemented as a method and that method is private, it's still a specified behavior. The way the code is currently laid out this would look something like:
describe Puppet::SSL::CertificateAuthority, "when initializing" do
it "should generate files" do
Puppet::SSL::CertificateAuthority.any_instance.expects(:setup_ca)
Puppet::SSL::CertificateAuthority.new
end
# ...
end
Implying that elsewhere we need to stub that method (because that's life with mocha). I would probably end up writing this like this:
describe Puppet::SSL::CertificateAuthority, "when initializing" do
before :each do
Puppet::SSL::CertificateAuthority.any_instance.stubs(:setup_ca)
end
it "should generate files" do
Puppet::SSL::CertificateAuthority.any_instance.expects(:setup_ca)
Puppet::SSL::CertificateAuthority.new
end
# ...
end
Of course, any_instance is a code smell, as you've already pointed out, though not in those exact words. I'd then focus on saying, "what is it about the code that makes me call this important instance setup method in initialize"?
I'm not a fan of big initialize methods, I'm not a fan of calling big setup methods in initialize (for precisely the reason you're pointing out -- it's hard to test them, which means it's hard to have written them test-first). I'm not at all opposed to taking the hint from difficulty in testing and seeing if I can't refactor code to alleviate the problem.
Some possible refactorings:
* The setup method may be a class method instead of an instance method, this could be part of the Indirection system lying in hiding (this writing to disk, etc., is that more properly indirected?).
* The setup method may actually be a class method on another class -- is there some sort of CA-related or storage-related class that's coming out? Think about whether Puppet::SSL::CertificateAuthority has one responsibility, and whether this setup code is part of that responsibility.
* Is it true that the setup must be called in the initializer (I take that "yes" is the answer, just asking for completeness' sake)?
There are some cases where there's no visible refactoring that cleans up the smell, in which case .any_instance might well be the way to go. I rarely ever have to use any_instance, though, granted, that's probably one of the side effects of being purely test-driven (or BDD, more accurately). To me this points to the any_instance need being a local solution for a non-local problem -- which is a reason it can be difficult to provide the best advice: most other people don't know the author's broader scope.
One thing that sticks out (to me) is that if you're specifying what that setup_ca method is doing, at least when you think about the behavior of the class, it sounds like you should be testing that behavior as well, which implies that it might start pulling towards being a public method instead of a private one. If so, it might end up being functionality on the class, or on a helper class. Hard to say, but working through getting more of that behavior unit-tested might expose what the tension is.
Of course, I happen to tend to agree with Jay quite often, and I tend to stick strongly to one-assertion-per-test. Lack of setup code is, I don't think, attainable. I read Jay as saying that he likes to see things at hand and he's willing to make his tests repetitive to get all the setup onto the code on the screen. I like before :each and friends, personally. I can see how they can be over-used with deeply nested describes. I can see how non-intuitive test helpers, much less test setup code in other files with big side-effects, can cause nightmares for someone trying to maintain the tests. So I'd say the less you have to do to setup for your tests the better, and the closer to the test that work is the better (and I'd advise treating big setup as a code smell as well).
Rick
Of course, I realize that I also have to test this setup behaviour -- the point was that it's a real pain to test methods called from initialize. My next step in the tests is to go through and test all of the different things done in setup; I've already got the behaviours described.
This is going to be an even bigger pain, of course; it's a very simple method (three lines of code, pointing to a total of about 20 lines of code in maybe 4-5 methods), but the fact that the setup method involves three steps (init key, init certificate request, init certificate) means that to test one I have to stub the other two, which is painful, annoying, and repetitive.
As to why the method is in initialize, the CA exists entirely to sign certificates, and I want to know at initialization whether it will be able to do that. The only way to do this is to fully set the instance up.
The CA is a special case in that all of its certificates are unique, and as such have unique locations -- they're not part of a collection, so I've chosen not to use the indirector to manage them. To do that, I'd need to create three files (terminus classes for keys, requests, and certificates), each of which would be a good bit different from their parent class because of the uniqueness, and the fact that the CA always writes its files out as a special user.
The reason I don't want to just make setup_ca a public method is that it's an implementation that doesn't count as a behaviour. That is, I need to make sure the various files are in place, but I don't care how it's done or what the method names are, and I want to be able to refactor without changing my tests.
And the initialize method is currently three lines long -- not what I would call a complicated method.
Tags
