opensoul.org

A common criticism of RSpec is that it is very verbose. I don’t necessarily agree (or care), but I thought it would be fun to see how concise I could make my specs.

Here are some simple specs from a client project:

describe Company do
  before do
    @company = Company.new
  end

  it "should have many classifications" do
    @company.should have_many(:classifications)
  end

  it "should have many industries through companies" do
    @company.should have_many(:industries, :through => :classifications)
  end

  it "should have many locations" do
    @company.should have_many(:locations)
  end

  it "should have many leads" do
    @company.should have_many(:leads)
  end

  it "should have many jobs" do
    @company.should have_many(:jobs)
  end

  it "should have many notes" do
    @company.should have_many(:notes)
  end

  it "should have many phones ordered by phone type position" do
    @company.should have_many(:phones, :as => :phonable,
      :include => :phone_type, :order => 'phone_types.position')
  end

  it "should have many events" do
    @company.should have_many(:events)
  end

  it "should have many interests" do
    @company.should have_many(:interests)
  end

  it "should belong_to an exchange" do
    @company.should belong_to(:exchange)
  end

  it 'should belong_to a primary_contact' do
    @company.should belong_to(:primary_contact, :class_name => 'Job')
  end

  it 'should have many articles' do
    @company.should have_many(:articles,
      :order => 'articles.date DESC, articles.created_at DESC')
  end
end

These specs check the declared associations on our company model using some custom matchers. They are not very complicated, but are somewhat repetitive. Each example has a description that is basically a duplication of the implementation.

Step 1: remove the description

For a while now, RSpec has had the ability for matchers to be self describing. If you don’t pass a block to #it, it uses the description provided by the matcher.

it do
  @company.should have_many(:jobs)
end

When that spec is run, it gives the output “should have a has_many association called :jobs”. Depending on what you’re speccing, the built in description isn’t always clear, but in this case it’s great.

See #simple_matcher if you want to create custom matchers with useful error messages.

Step 2: remove the subject

So the duplication within each example is gone, but if you look at the full spec above, each example calls @company.should. Accessing an instance variable isn’t what I would consider “duplication”, but thanks to a nifty new feature added to RSpec today, it’s now unnecessary noise. We can simply call #should within our example, and it will use a new instance of the described type as the “subject”.

describe Company do
  it do
    should have_many(:jobs)
  end
end

You can customize the subject if you don’t simply want a new instance.

describe Company, 'validations' do
  subject { Company.new(valid_company_attributes) }

  it do
    should be_valid
  end
end

Note: As David Chelimsky points out in the comments, this is not released yet and is subject to change.

Step 3: One-liner

Lastly, we can use the one line block:

describe Company do
  it { should have_many(:classifications) }
  it { should have_many(:events) }
  it { should have_many(:interests) }
  it { should have_many(:jobs) }
  it { should have_many(:leads) }
  it { should have_many(:locations) }
  it { should have_many(:notes) }
  it { should have_many(:articles, :order => 'articles.date DESC, articles.created_at DESC') }
  it { should have_many(:industries, :through => :classifications) }
  it { should have_many(:phones, :as => :phonable, :include => :phone_type, :order => 'phone_types.position') }
  it { should belong_to(:exchange) }
  it { should belong_to(:primary_contact, :class_name => 'Job') }
end

That’s pretty sexy. I wasn’t able to do this with all of the specs in my app, but it worked with quite a few of them.

All the cool kids tell me that fixtures just aren’t the “in” thing anymore. Speaking of which, when did fanny-packs stop being cool? I wish someone would have said something…

Anyway, while I still haven’t fallen out of love with (foxy) fixtures, I have taken some of the home-grown alternative methods for a spin, but so far, I’ve been slightly frustrated with my experience.

The predominant replacement seems to be the “factory” pattern, talked about by Dan Manges and then followed up by the FixtureReplacement plugin. The basic idea is that all your tests/specs should setup the data that they require, and there is a factory that makes it easy to create the necessary valid test data. For example, a homegrown solution might look like this:

class Generate
  def self.person(options={})
    p = Person.create!({
      :name     => 'Brandon',
      :login    => 'brandon',
      :password => 'testing'
    }.merge(options))
    p.account ||= Generate.account
  end
end

describe Person, '#authenticate' do
  it 'should return the person record if successful' do
    person = Generate.person
    Person.authenticate(person.login, 'testing').should == person
  end
end

This method scales pretty well. There are some issues, such as the use of create!. Sometimes you intentionally want an invalid record, or you want a new record with just some valid attributes initialized. It’s also slower than fixtures, but I don’t care too much about that right now.

But there’s a bigger issue. Like a good little BDDer, I have also been stubbing and mocking all my “unit tests” so there is no interaction with the database or code outside of what is being tested. To make the factory method work, I now need to define 3 different methods depending on what I’m testing: one using #create!, one using #new, and another one using stubbing.

What do we do about it?

I think the FixtureReplacement plugin is on the right track. It handles the new vs. create problem very nicely.

module FixtureReplacement
  attributes_for :person do |u|
    u.name     = String.random
    u.login    = String.random
    u.password = String.random
    u.account  = default_account
  end
end

@person = new_person(:login => 'brandon')
@person = create_person

Before I heard about the FixtureReplacement plugin, I actually concocted my own little solution that handles all three scenarios. I’m not crazy about the syntax, but it works for my needs.

Generate.add Person, :name => 'Brandon', :login => 'brandon' do |generator, u|
  u.account ||= generator.account
end

@person = Generate.person(:login => 'brandon')
@person = Generate.new_person
@person = Generate.stub_person

Anywho, for the fixture-less approach to work, I think stubs need to be supported.

Is all this really worth it?

That’s the question I find myself asking. What’s wrong with fixtures anyway?

One thing I really like about fixtures, when done right, is that they help tell the story of your application. You get to know the fixture data as you work on the app.

The factory method also seems to go against convention-over-configuration. Instead of having a default “configuration” when your tests run, you have to configure each test. Call me lazy, but that just seems like too much work.

I find that testing takes a lot more effort now than it did with fixtures. And as a result, I’m more hesitant to test everything. So while the factory method and stubbing is theoretically supposed to help you test better, I feel like they’ve had the opposite effect.

What do you think? Have you had success using the factory pattern for tests?

The RSpec team is hoping to move to some form of distributed version control, giving both git and Mercurial a test run. This month, they’re checking out git.

You can clone the git repository using:

git clone git://gitorious.org/rspec/mainline.git rspec

I ran across this issue several weeks ago, but it came up at the latest Grand Rapids Ruby Group meeting, so I thought I would share it.

Ruby’s require, for better or worse, doesn’t expand paths. As the docs point out, require 'a'; require './a' will load a.rb twice. This doesn’t matter most of the time, but there’s one place it’s used often that will bite you: tests and specs.

Every Rails’ test has a line similar to this:

require File.dirname(__FILE__) + '/../test_helper'

This doesn’t normally cause any problems, as long as every test has an identical require statement. Where you start to get into problems is when you have tests in a nested subdirectory (like test/controllers/admin/users_controller_test.rb), in which case the require statement would look like this:

require File.dirname(__FILE__) + '/../../test_helper'

require sees this as a different file and will re-load it. This still shouldn’t hurt you unless you’re doing something in test_helper.rb that would be changed by loading it twice (like aliasing a method). This also effects RSpec with requiring spec_helper.rb

The solution? Expand the path yourself.

require File.expand_path(File.dirname(__FILE__) + '/../../test_helper')

It’s not really clear to me why Ruby’s require works this way. You would think a method that was intended to only load a file once would make sure that it never re-loaded the same file, no matter how it was referenced. It definitely doesn’t adhere to the “principle of least surprise”. Any idea why?

The theory is that tests are supposed to be agnostic of the implementation. This leads to less brittle tests and actually tests the outcome (or behavior).

With RSpec, I feel like the common approach of completely mocking your models to test your controllers ends up forcing you to look too much into the implementation of your controller.

Here is an example of a spec that is generated by the rspec_scaffold generator:

describe ThingsController, "handling POST /things" do

  before do
    @params = {}
    @thing = mock_model(Thing, :to_param => "1", :save => true)
    Thing.stub!(:new).and_return(@thing)
  end

  def do_post
    post :create, :thing => @params
  end

  it "should create a new thing" do
    Thing.should_receive(:new).with(@params).and_return(@thing)
    do_post
  end

  it "should redirect to the new thing" do
    do_post
    response.should redirect_to(@thing)
  end
end

This by itself is not too bad, but the problem is that it peers too much into the controller to dictate how the model is used. Why does it matter if my controller calls Thing.new? What if my controller decides to take the Thing.create! and rescue route? What if my model has a special initializer method, like Thing.build_with_foo? My spec for behavior should not fail if I change the implementation.

This problem gets even worse when you have nested resources and are creating multiple models per controller. Some of my setup methods end up being 15 or more lines long and VERY fragile.

RSpec’s intention is to completely isolate your controller logic from your models, which sounds good in theory, but almost runs against the grain for an integrated stack like Rails. Especially if you practice the skinny controller/fat model discipline, the amount of logic in the controller becomes very small, and the setup becomes huge.

So what’s a BDD-wannabe to do? Taking a step back, the behavior that I really want to test is not that my controller calls Thing.new, but that given parameters X, it creates a new thing and redirects to it.

Here’s the approach I’ve been taking:

# spec_helper.rb
def valid_thing_attrs(attrs = {})
  {:name => 'Foo'}.merge(attrs)
end

# things_controller_spec.rb
describe ThingsController, "handling POST /things" do

  def do_post
    post :create, :thing => valid_thing_attrs
  end

  it "should create a new thing" do
    lambda { do_post }.should change { Thing.count }.by(1)
  end

  it "should redirect to the new thing" do
    do_post
    response.should redirect_to(@thing)
  end
end

My spec is now testing that posting certain attributes creates a new thing (in the database) and redirects to it. You’ll notice that I have a method called valid_thing_attrs. I don’t remember where I picked up this pattern, but it is something that has allowed me to minimize all of my test’s dependencies on the model.

My spec is now somewhat dependent on my model–and thus will break if my model gets hosed–but that is a small price to pay, in my opinion, for an implementation-agnostic spec with significantly less overhead.

What do you think?

I am hanging out in the lobby of our hotel at RailsConf as David Chelimsky and Aslak Hellesøy are releasing RSpec 1.0. Congrats to them and everyone else involved with the project!

One of the things I always check when spec’ing is that the model that I just created is valid.

@mymodel.should be_valid

This uses RSpec’s predicate support (any call to be_something? calls something? on the target object), and returns a correct but very unhelpful error:

expected valid? to return true, got false

Here is a little rspec matcher that I threw together this morning to check if a model is valid and spit out the validation errors:

module Spec
  module Rails
    module Matchers
      class BeValid  #:nodoc:

        def matches?(model)
          @model = model
          @model.valid?
        end

        def failure_message
          "#{@model.class} expected to be valid but had errors:\n  #{@model.errors.full_messages.join("\n  ")}"
        end

        def negative_failure_message
          "#{@model.class} expected to have errors, but it did not"
        end

        def description
          "be valid"
        end

      end

      def be_valid
        BeValid.new
      end
    end
  end
end

Now, I get:

MyModel expected to be valid but had errors:
  Password confirmation can't be blank
  Email has already been taken

This is nothing fancy, but it’s the small stuff that make life meaningful, so I thought I would share it. Throw the matcher code above into your spec_helper and check if your models are valid.

update: added negative_failure_message so “model.should_not be_valid” will work