Testing Cached Values

I'm currently in the middle of the largest refactoring effort I've ever done, while simultaneously learning tons about how to be a better developer. I'm constantly feeling a bit overwhelmed, a bit behind the curve, and like someone's going to look at my code one day and say, "hey nimwit, you just pull this string here and suddenly 2/3 of your code just goes away."

However, one thing I've really grokked recently is that if you're fighting your tools too much, you are on the wrong track, and one way in which it seems I'm constantly fighting my tools is the combination of cached values and testing.

For instance, I have an HttpPool class that knows how to set up Net::HTTP instances with all of the SSL information they need. This class caches SSL information like the certificate and key, so that each connection doesn't hit the disk for this info, which is obviously a pretty decent use of the cache. This caching generally takes this simple form:

def ssl_host
    unless defined?(@ssl_host)
        @ssl_host = Puppet::SSL::Host.new
    end
    @ssl_host
end

Yes, you could just do @ssl_host ||= Puppet::SSL::Host.new, but I've gotten some weird behaviour out of that in the past, and it also throws a warning for undefined variables.

So anyway, this works fine in unit tests; I test the caching, and I use mocks everywhere else. When I get to integration tests, though, it starts to really hurt, especially since I try not to do much mocking in my integration tests.

For instance, say I've got two unrelated tests that do an ssl connection. They each create some certificates, start a daemon, and try to connect. In this situation, the first one caches the ssl information, and the second one uses the cached values instead of its own new certificate, and you get invalid certificates.

After talking to Rick Bradley on #nashdl on IRC (gotta represent!), I've decided on at least an initial course of action. I'm going to create a module that provides both a caching and cache-clearing interface; anyone using cached values would use this caching interface instead of caching the values themselves, and the module itself would give you a single point of entry for clearing all caches on the system.

My first instinct was to create a Cache class or struct and keep a list of them in the caching module, so they can be cleared as necessary, but my recent work with TTLs has made me realize that time-based concepts of cache dirtiness are much better than actively cleaning.

So now, I'm thinking that the caching module will just have a timestamp, and only cached values created after that time will be valid. Before the Cache struct returns any values, it will always check that time, and it will know whether the value it has is still good or should be discarded.

This keeps us from maintaining global lists of caches, and it also makes clearing caches insanely cheap -- just reset a timestamp. Given that Puppet already sometimes has onerous memory requirements, I also like that it makes the caches themselves more likely to get garbage collected, since only the caching instance ever knows about the actual cache itself.

So my new method would look something like this:

def ssl_host
    cache(:ssl_host) { Puppet::SSL::Host.new }
end

That bit with the block is something I just thought of; if we've got a value, it's not called, but if we need a value, it's there for us. Pretty sweet.

Now just to implement it.

Wed, 07 May 2008 | Tags: programming, testing, caching

Jay and I converge on testing

Those four people who have been reading this blog for a while know I've been struggling to think and program like Jay Fields. In particular, he seems to have presented a few rules in the past that don't like to be used together:

Inline setup code: You apparently should not use before or setup blocks with your code; instead, you should have all of the setup code in every method.
Mocks instead of Stubs: Here he claims that it's always better to use expects instead of stubs.
One expectation per test: With only one expectation, you're only testing one feature at a time, and you're guaranteed that each behaviour is tested separately.

Now, let's do a simple combinatorial exercise, and put these three rules together:

You should have all related code in your test
You should only use Mocks (expects), not Stubs (stubs)
You should only have on expectation (expects) per test

It's pretty clear that, like the old saw about programming ("All programs can be reduced to one line of code with a bug in it"), Jay is pointing us toward tests that can largely only be one line of code. Yeah, I know sometimes setup methods don't involve any mocking, but often they do.

And, since your tests can only be one line of code, they can't test very much, which means that all of your methods need to also be one line of code, else they aren't testable. (Yes, I'm being a touch extreme here, but that is where the arrow is pointed, anyway.)

You can see how this would kinda drive me bonkers. Some local dev friends have been trying to help me see the error of my ways, mostly so my code would stop looking like such crap; I've learned a helluva lot in the last 8 months or so, and most of it has actually made my code look more like Jay would recommend. I will say it's blindingly obvious Jay is doing internal development at enterprises, rather than developing as part of a consistent team producing software that is distributed to the wider world.

But one can only go so far, and the three rules above, in combination, are just way too far. I've often kind of sputtered expasperatedly at Jay's posts, especially his announcement of his new testing tool, expectations. Again, I can kind of see where he's going with that, but you've got another thing coming if you think I'm using it, especially given how happy (at least, relative to test/unit) I am with RSpec.

Also, I think it's just stupid having all setup code inline. DRY ("don't repeat yourself") is just as true in your test code as anywhere else, and having a maintainable test code base is, IMO, more important than having your normal code base be maintainable, because tests are kind of unnecessary. If you have good, readable, maintainable tests, then people who contribute will also contribute tests. If your tests are all 50 lines long and have lots of repetition, then 1) you've got 5x the amount of code you should, which is wicked expensive, and 2) you've got so much code no one will look at it. Yay, never getting patches with tests in them. My favorite example of this is Steve Yegge's rant Code's Worst Enemy; he describes his 500k line Java project with no tests, which is a lot of code but much less code than if it had tests. I've experienced in Puppet that test code seems to be much harder to maintain that normal code (although maybe it's just own crap test code, not normal test code), and having 5x test code than normal code would make me just quit writing unit tests entirely.

So, I am absolutely overjoyed to announce that Jay has changed one of his rules: He now recommends stubs over mocks. This is clearly just for setup code and such, but it's a big step. He even goes into using stub_everything, which I find is the only way to build tests that aren't fragile. For instance, say you start with this code:

class MyClass
    def go
        start()
        finish()
    end
end

describe MyClass do
    before
        @me = MyClass.new
        @me.stubs(:start)
        @me.stubs(:finish)
    end

    it "should start when going" do
        @me.expects(:start)
        @me.go
    end

    it "should finish when going" do
        @me.expects(:finish)
        @me.go
    end
end

Now you find you need a validation method, so you add this test:

it "should validate when going" do
    @me.expects(:validate)
    @me.go
end

Update: Fixed code to actually call @me.go in the validate test.

Oops. Now your single test passes, but your two old tests break, because you were only stubbing start and finish, instead of using stub_everything. Your setup code needs to be modified to take this new call into account (or, if you're Jay, you need to modify every test in your suite; yay). This comes up constantly. If you specifically mock or stub methods during setup, then you are almost guaranteed to have cascading failures when you expand your code.

Anyway, the point is, if I tried to follow Jay's rules, then the above trivial change -- I add one line of code to a very simple method -- would result in me adding a test for that line, plus at least one line of code in every other test in that suite. Instead, if I use stub_everything, then I add my new test and I'm done. (Well, kind of; notice I'm not actually testing the order of the method calls, which is actually pretty tough.)

My recommendation is to read Jay, since he's clearly thinking and talking about aspects of testing that not many others are, but read him with a skeptical eye, and be willing to say "That's just nuts!" and write your own relatively abstracted test code. And if you're working with people who can't think to look at their setup code when a test fails, then you need to find a different job.

Tue, 06 May 2008 | Tags: ruby, testing, programming, bdd, tdd, jayfields

Testing Initialization Code

One of the things I continually struggle with in testing is code that runs during initialization. A lot of times this code is very simple:

def initialize(name, cert, key)
    raise Puppet::Error, "Cannot manage the CRL when :cacrl is set to false" if [false, "false"].include?(Puppet[:cacrl])

    @name = name

    unless read(Puppet[:cacrl])
        generate(cert, key)
        save(key)
    end
end

It's easy to test that first and second line, and it's entirely obvious what that last bit does, but it's fantastically difficult to test, especially if you follow the advice of Jay Fields and try to stick to one expectation per test.

If you do it all in one test, you end up with a relatively long test that covers a single specific version, but it doesn't describe the behaviours very well. You want something like this:

describe "when initializing" do
    it "should fail if :cacrl is set to false"

    it "should set the name"

    it "should read the crl in from disk"

    describe "and no crl exists on disk" do
        it "should generate a new crl"

        it "should save the new crl"
    end
end

The only way to do this, though, is to use stub_everything, and then individually test for each method, which is messy.

Even worse, you now have to stub out these methods every time you want to test an instance of the class in any other way. For instance, (as you might have guessed) I'm remodeling our Certificate Revocation List as a class, and I'm going to need to test the actual revocation, along with storage to disk. Each of these are made more complicated by the code in the initialize method.

Why, then, don't I just leave the code out?

Well, I could easily have it lazy evaluate, only running when someone actually asks for the crl. The problem is that I've consistently found that lazy evaluation causes more problems than it saves. I tend to run into permission problems (because the code doesn't evaluate until puppetmasterd is running as puppet, when it sometimes doesn't have the permissions it needs), and it's just very difficult to really control ordering.

Also, it just feels messy to reorganize easy code to make it more testable. There seems to be a postulate in the testing world that code that's difficult to test is bad code, but I defy anyone to argue that the above code is unclear or "bad code", other than just directly saying it's bad because it's hard to test.

I expect that in this case I'll have a generate_and_save method that, well, generates and saves, or maybe a load_or_create method that does this bit. Yay. Because simple code is hard to test, I end up with less simple code, and I still have to use stub_everything.

Tue, 01 Apr 2008 | Tags: ruby, testing, initialization, programming

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

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

A little more RSpec

I've been using RSpec more thoroughly recently, so I figured it's time produce a post covering some of the things I've learned.

Particularly, thanks to much forcing by Rick, I have now come to realize that RSpec involves writing two very coupled and very important documents: The spec, which describes how your code should behave, and the tests, which are attached to the spec and theoretically prove that your code does behave this way.

This was a big step in realization for me, because I had always thought of my tests as just that: tests. That is, their job was to make sure my code was behaving as I thought it should be behaving. When working with Rick the other day, though, he kept saying my spec was bad and I kept wanting to shout that I understood my tests were bad, let's just fix them.

After enough prodding, though, and some judicous using of RSpec's formatting, I came to see that he views them as separate things, and that I should too.

With RSpec, you can obviously run your tests:

luke@phage(0) $ bin/spec unit/indirector/indirector.rb
.........

Finished in 0.010605 seconds

9 examples, 0 failures
luke@phage(0) $

But you can also run them and produce a document that looks suspiciously like a spec:

luke@phage(0) $ bin/spec unit/indirector/indirector.rb  --format s

Puppet::Indirector when available to a model
- should provide a way for the model to register an indirection under a name

Puppet::Indirector when registering an indirection
- should require a name when registering a model
- should create an indirection instance to manage each indirecting model
- should not allow a model to register under multiple names
- should make the indirection available via an accessor

Puppet::Indirector when redirecting a model
- should give model the ability to lookup a model instance by letting the indirection perform the lookup
- should give model the ability to remove model instances from a terminus by letting the indirection remove the instance
- should give model the ability to search for model instances by letting the indirection find the matching instances
- should give model the ability to store a model instance by letting the indirection store the instance

Finished in 0.014107 seconds

9 examples, 0 failures
luke@phage(0) $

Note that it's still running the tests here (as shown by the lack of failures at the bottom), it's just producing output differently.

I was surprised to find Rick wanting to read this output again and again, completely ignoring any associated test code. It finally hit me, of course, that if we aren't doing a good job of describing the expected behaviour, then how can we do a good job of testing it?

When we first started working through some code recently, he kept niggling about some code that wasn't in the spec, which kinda miffed me because it was clearly being tested. When I finally realized what he meant about the difference between the spec and the tests, no matter how tightly coupled they are, I realized why it mattered so much to him: There was behaviour in my code that I essentially hadn't documented, even if I was confident I was testing that it was working.

Certainly an interesting realization.

Sun, 23 Sep 2007 | Tags: ruby, rspec, puppet, development, programming, testing, bdd

Using test/unit-style setup/teardown in RSpec

I've been starting to use RSpec within Puppet recently, and one of the things that really annoys me about RSpec is that it uses hooks for setup and teardown instead of methods:

describe "My stuff" do
    before do
        ...
    end

    it "should ... "

    after do
        ...
    end
end

Sure, it's just as functional in the simple cases, but the simple cases don't go very far when you've got 50k lines of test code. Specifically, this makes it stupidly difficult to have modules that include modules, all of which have their own setup and teardown.

However, after reading through the RSpec code today, I found a workaround. You can provide hooks in the RSpec configuration that add new before and after hooks for all of your tests:

Spec::Runner.configure do |config|
  config.mock_with :mocha
  config.prepend_before :each do
      setup() if respond_to? :setup
  end

  config.prepend_after :each do
      teardown() if respond_to? :teardown
  end
end

Now I have test/unit-style setup and teardown, which, importantly, means I can easily use my existing test modules (since I'm in the process of converting my tests from test/unit to RSpec), and it also means that those modules can include other modules, or a given behaviour can include multiple modules, and all of the modules' hooks will get called.

Whew!

Tue, 11 Sep 2007 | Tags: ruby, rspec, testing, test/unit

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. :)