Another Testing Conundrum: Mocks or Real Objects?
So, I have a very simple method for generating a certificate request:
# How to create a certificate request with our system defaults.
def generate(key)
Puppet.info "Creating a new SSL certificate request for %s" % name
csr = OpenSSL::X509::Request.new
csr.version = 0
csr.subject = OpenSSL::X509::Name.new([["CN", name]])
csr.public_key = key.public_key
csr.sign(key, OpenSSL::Digest::MD5.new)
raise Puppet::Error, "CSR sign verification failed; you need to clean the certificate request for %s on the server" % name unless csr.verify(key.public_key)
@content = csr
end
Short, readable, clear. Theoretically, this should be easy to test, too, and hopefully the tests should be about as short.
If you follow the strategy of using a mock request, your first pass ends up looking disappointingly like this:
describe "when generating" do
before do
@instance = @class.new("myname")
key = Puppet::SSL::Key.new("myname")
@key = key.generate
@request = mock 'request'
OpenSSL::X509::Request.expects(:new).returns(@request)
@request.stubs(:version=)
@request.stubs(:public_key=)
@request.stubs(:subject=)
@request.stubs(:sign)
@request.stubs(:verify).returns(true)
end
it "should log that it is creating a new certificate request" do
Puppet.expects(:info)
@instance.generate(@key)
end
it "should set the subject to [CN, name]" do
subject = mock 'subject'
OpenSSL::X509::Name.expects(:new).with([["CN", @instance.name]]).returns(subject)
@request.expects(:subject=).with(subject)
@instance.generate(@key)
end
it "should set the version to 0" do
@request.expects(:version=).with(0)
@instance.generate(@key)
end
...
Obviously, the test isn't done yet. Total line count for a 13 line method? 66 lines. (Admittedly, I'm a bit generous on white space.)
The thing I dislike most about this test is that the setup code has to know exactly how many tests there are -- if I add a new setting to the request, then I need to change my setup code, which has bitten me multiple times and is a real pain. One option for removing that is to skip the mock:
describe "when generating" do
before do
@instance = @class.new("myname")
key = Puppet::SSL::Key.new("myname")
@key = key.generate
@request = OpenSSL::X509::Request.new
OpenSSL::X509::Request.expects(:new).returns(@request)
@request.stubs(:verify).returns(true)
end
...
I still need to stub the verify method, because of the check that I do on the result, but skipping the use of mocks makes it simpler.
Note that I've been using a real SSL key the whole time -- this slows things down a little, since the ssl keys aren't free to make, but it simplifies my setup code in the same way that the above setup is simpler than the top one.
Of course, a third option is to keep the mock but use stub_everything. This is apparently another indication of code smell, and it seems the difference between this and just using a real object is that I need to write a separate integration test if I use a mock, but with this, I don't really.
I think I'm going to keep this last setup method. It's still 65 lines of code for a 13 line method, but at least it lists each behaviour separately. My previous test included this behaviour:
# It just doesn't make sense to work so hard around mocking all of this crap five times in order to get this test down to one expectation
# per test.
it "should create a new certificate request with the subject set to [CN, name], the version set to 0, the public key set to the privided key's public key, and signed by the provided key" do
@request = mock 'request'
OpenSSL::X509::Request.expects(:new).returns(@request)
subject = mock 'subject'
OpenSSL::X509::Name.expects(:new).with([["CN", @instance.name]]).returns(subject)
@request.expects(:version=).with 0
# For some reason, this is failing, even though the values are correct.
# It seems to be considering the values different if i use 'with'.
@request.expects(:public_key=)
@request.expects(:subject=).with subject
# Again, this is weirdly failing, even though it's painfully simple.
@request.expects(:sign)
@request.stubs(:verify).returns(true)
@instance.generate(@key).should == @request
end
I'm not fond of this, because it doesn't list each behaviour separately, but it seemed to be appropriate for such a simple method.
Unfortunately, the 'verify' method (assuming I didn't use a mock) was nearly as long as this method, since it has to stub everything.
I'm thoroughly convinced that this generate method is reasonable, but the rules of the game are that it's got code smell because it's difficult to test, which I don't buy at all. So now I've got 5:1 tests to code for a really simple and maintainable method, meaning I'm basically doomed to always spend more time on the tests than the functionality, which is utter crap.
Wed, 12 Mar 2008 | Tags: ruby, mock, mocha, testing
If a method has 5 behaviors it should have at least 5 specs. This is why when lots of specs pop up on a class we say there's a "code smell" -- the method and/or the class are probably doing too much.
Fwiw, we've got Rails applications where the amount of test code vs. application code is more than 12:1. At that point, though, we're taking copious advantage of rspec's shared behaviors. To be specific, in Rails we're saying a controller "behaves like a RESTful controller", for example, which triggers a cascade of behavior specs for that controller. There can be a lot of behavior in a system, and an exponential (not quadratic, which is what most people mean by "exponential", but exponential) amount of behavior when integrating across the entire system. That means a lot of tests, on the surface, it means that aggressively managing complexity (in code and tests) is a requirement.
Specifically here, one of the problems with the generate(key) method is that it's resting above an un-refactorable OpenSSL class hierarchy. If you could refactor OpenSSL that method would (first pass) probably read like this:
def generate(key)
@request = OpenSSL::X509::Request.new(:key => key, :name => [["CN", name]])
end
and you would end up with a handful of specs testing the error condition(s), whether the instance variable was set properly, and whether the request was returned.
Another question is whether this method is in the right class at all (I can't answer that without knowing how it's used, which is a matter of being mindful of the behaviors of the class and the behaviors of the callers of the method). Should key be a method or an instance variable (hence accessor method) on the class which houses #generate? Not sure, but I'd be looking there. Is there a key class that's missing that should handle a delegation instead of passing the key around? Not sure.
Again, the thing about testability, code smells, and refactoring is that the concerns are mostly non-local. Sure the method might be "simple and maintainable" when viewed in isolation (I argue that the above method is simpler and more maintainable but you're hamstrung by non-local concerns, at least at first glance), but we're building systems and not simply methods. Systems are non-local by definition, and code smells come from non-local concerns.
At some point, if one deals with an externality like OpenSSL one eventually runs into the problem that OpenSSL is not well factored, and that you have little control over that. The right way, in my opinion, to handle it is to wrap the externality in an abstraction that simplifies it. At that boundary layer (which #generate is an example of) things are often going to be smelly. Try your best. Send an email to the maintainers of OpenSSL if you want; or write a new wrapper facade for OpenSSL that does reasonable things. Or just move on.
Rick
Tags
