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

Fake Ruby Test Classes

Now that I mostly have providers done, I can start taking advantage of it for testing.

For instance, I now have user testing split into two chunks -- providers and types. The provider testing verifies that all of the methods actually do what they're supposed to -- calling shell= actually changes the user's shell, etc.:

def attrtest_shell(user)
    old = current?(:shell, user)

    newshell = findshell(old)

    unless newshell
        $stderr.puts "Cannot find alternate shell; skipping shell test"
        return
    end

    assert_nothing_raised {
        user.shell = newshell
    }

    assert_equal(newshell, current?(:shell, user),
        "Shell was not changed")

    assert_nothing_raised {
        user.shell = old
    }

    assert_equal(old, current?(:shell, user), "Shell was not reverted")
end

(This isn't named test_<something> because a different method detects this method and calls it dynamically.)

This modifies the actual state on the machine; the current? method is abstracted a bit so it works with the builtin Etc ruby module and knows how to look in NetInfo (I can't seem to get it to flush its cache fast enough to use the Etc library reliably on OS X).

So, I've got one set of tests that know how to verify that when I call provider.shell = "/bin/sh" it does the right thing, which means I don't need to test that anywhere else. Thus, when I test my types, I just want to test that the method gets called correctly, I don't need to test what happens inside the method.

So, I create a base class for fake providers:

class FakeProvider
    attr_accessor :model
    class << self
        attr_accessor :name, :model
    end

    # Set up methods to fake things
    def self.apimethods(*ary)
        @model.validstates.each do |state|
            ary << state unless ary.include? state
        end
        # Make accessor methods for everything
        attr_accessor *ary
    end

    # Our fake providers are always suitable
    def self.suitable?
        true
    end

    def initialize(model)
        @model = model
    end
end

Then we create a sub-class for user type testing:

p = Puppet::Type.type(:user).provide :fake, :parent => TestPuppet::FakeProvider do
    @name = :fake
    apimethods
    def create
        @ensure = :present
        @model.eachstate do |state|
            send(state.name.to_s + "=", state.should)
        end
    end

    def delete
        @ensure = :absent
        @model.eachstate do |state|
            send(state.name.to_s + "=", :absent)
        end
    end

    def exists?
        if defined? @ensure and @ensure == :present
            true
        else
            false
        end
    end
end

FakeUserProvider = p

The apimethods call in this subclass causes accessor methods to be created for every state on users, e.g., gid, gid=, home, home=, etc. If this isn't appropriate for your class, you can instead pass in a list of methods to add (e.g., apimethods :mymethod, :othermethod).

The only other step is to mark this as the default provider in your setup code:

def setup
    super
    Puppet::Type.type(:user).defaultprovider = FakeUserProvider
end

def teardown
    # Reset it, so that the system will instead discover the "correct"
    # provider.
    Puppet::Type.type(:user).defaultprovider = nil
    super
end

Now you can test your type without actually modifying the system at all. In this case, if we start with a missing user, set some data up, and then create the user, our provider should have all of the data stored in instance variables (whereas a normal provider would modify a real user).

This is a simplistic, "just make sure the basics are working" test method:

def test_simpleuser
    name = "pptest"

    user = nil
    assert_nothing_raised {
        user = Puppet.type(:user).create(
            :name => name,
            :comment => "Puppet Testing User",
            :gid => Process.gid,
            :shell => findshell(),
            :home => "/home/%s" % name
        )
    }

    assert(user, "Did not create user")

    comp = newcomp("usercomp", user)

    trans = assert_events([:user_created], comp, "user")

    assert_equal(user.should(:comment), user.provider.comment,
        "Comment was not set correctly")

    assert_rollback_events(trans, [:user_removed], "user")

    assert(! user.provider.exists?, "User did not get deleted")
end

A lot of this uses methods I've written in my test system (e.g., assert_events, which verifies specific events are generated and thus specific work was done), but the important point here is that I can start with a missing user, "create" the user, then verify that the provider (which, again, would normally interact with /etc/passwd or netinfo, but in this case just uses instance variables) has the right data.

Actually, that's not right -- the important point is that I can test all of this without actually modifying the system. Before I created this split between types and providers, I had to run this test as root because it had to modify the system and compare against the system; now I can run this as a normal user because it never touches the system.

Now that's progress. :)

Thu, 10 Aug 2006 | Tags: puppet, ruby, testing, mock