RSpec helped me refactor my code.
I’ve been extremely against using RSpec. I always found it rather clunky, but it turns out that resources to really help a person learn how RSpec works are dificult to find. The examples you find out on the web are just poorly written or just contrived and impractical, or they’re so hopelessly overengineered that a newcomer would be overwhelemed.
This weekend I took it upon myself to really learn RSpec and so I started rewriting some of the tests for FeelMySkills. I started with the Account model which is used all over the app. The account_id is stored in the session and I use the Restful_authentication plugin to get access to a current_account method which returns the Account object. I want to be able to determine whether or not that account is an Admin, and I have an entry in the roles called “admin”. Nothing too special about all this, as many apps use a similar bit of functionality.
To make this easy on myself, I wrote a method called is_admin? which returns true if the admin role is associated with my account and nil if it’s not, and as we all know, nil evaluates to false, and anything other than nil or false evaluates to true.
When an account is created, I give them a role called “user”. Eventually, Pro users will have a different role, giving them access to more stuff in the system. Here’s what i have so far:
class Account < ActiveRecord::Base
has_and_belongs_to_many :roles
after_create :add_user_role
def is_admin?
self.roles.detect{|r| r.name == "admin"}
end
def is_user?
self.roles.detect{|r| r.name == "user"}
end
def add_user_role
self.roles << Role.user
end
end
So, when I create a new user, I need to make sure that user gets the user role. My original Test/Unit test looked like this:
def test_new_account_should_have_user_role account = Account.create(:login => "test", :password=>"test", :password_confirmation => "test", :email => "test@test.com") assert account.is_user? end
This test passes without any issues, so I know the code is right. Here’s what I tried with RSpec:
describe "when creating an account" do fixtures :accounts, :roles before(:each) do @account = Account.create(:login => "test", :password=>"test", :password_confirmation => "test", :email => "test@test.com") end it "should have the user role" do @account.is_user?.should be_true end end
Imagine my surprise when I ran this spec and it failed! The reason why makes perfect sense when you start thinking about it.
First, RSpec’s matcher be_true evaluates the response of the method to be equal to true. The code for is_admin? actually returns an instance of Role, and not true like I asserted earlier. While that method evaluates to true, it does not equal true. So it’s interesting that the assert method has no probelm making the evaluation, but RSpec’s matchers are pickier.
A fair argument here would be “why does your is_admin? method return a Role and not just true or false?” The answer is that I’m lazy. I rely on Ruby to work for me, and until now, #detect has been a great ally. In my controller code, I can do
if current_user.is_admin...
and all is well, without the need to explicitly return true or false from the is_admin? or is_user? methods.
A better way
Looking at the spec again, I notice that I am in fact asking for the role in that specification. So I rewrite it to grab the User role from the fixtures and ensure they’re equal and it passes.
it "should have the user role" do @account.is_user?.should equal roles(:user) end
But something about that bothers me. What am I really testing? I’m testing to make sure that the account is a regular user. Maybe I really do need a method that returns true or false.
It turns out that if you have a method in your model that ends with a question mark (?) and returns true or false, then RSpec can dynamically create a matcher for it. I rewrote the spec like this:
it "should be a user" do @account.should be_a_user end
and then added this method to my model:
# calls is_user? and returns true if is_user? returns a result, # or false if it returns nil def user? self.is_user? != nil end
and I ended up with something I am much more comfortable with. I think future refactorings might change this around even more, but I found this exploration to be extremely enlightening.
P.S. For those that are interested, I actually have several roles in my system and I don’t manually declare these methods like is_admin? and is_user? by hand. I use this instead:
class Account < ActiveRecord::Base
# ...
# constant containing all of the role names
# in the system
Role::ROLE_NAMES.each do |r|
class_eval <<-CODE
def is_#{r}?
self.roles.detect{|role| role.name == "#{r}"}
end
def #{r}?
self.is_#{r}? != nil
end
CODE
end
#...
end
That way I don’t need to add new methods when I implement factchecker or pro or business roles later. Just thought I’d share that.
