Wednesday, December 27, 2006

Rails: Unit Testing ActiveRecord Validations

There are at least 2 easy ways to unit test the validations of an ActiveRecord::Base subclass.

The most straight-forward way is to create a model and use the valid? method to determine if it is valid or not. For an example I'm going to reuse the PhoneNumber class I defined in a previous entry on ActiveRecord Unit Testing. The following code shows the three tests I've written to ensure that I've correctly defined a validates_presence_of for digits.
class PhoneNumberTest < Test::Unit::TestCase
Column = ActiveRecord::ConnectionAdapters::Column

test "digits are required to be valid" do
PhoneNumber.stubs(:columns).returns([Column.new("digits", nil, "string", false)])
number = PhoneNumber.new(:digits => "1234567890")
assert_equal true, number.valid?
end

test "invalid with no digits " do
PhoneNumber.stubs(:columns).returns([Column.new("digits", nil, "string", false)])
number = PhoneNumber.new()
assert_equal false, number.valid?
end

test "errors include digits on invalid digits" do
PhoneNumber.stubs(:columns).returns([Column.new("digits", nil, "string", false)])
number = PhoneNumber.new()
number.valid?
assert_equal "can't be blank", number.errors.on('digits')
end

end
After adding the validates_presence_of call to the PhoneNumber class, all the above tests pass.
class PhoneNumber < ActiveRecord::Base
validates_presence_of :digits
end
This all works fine, but I'm not sure it's necessary to actually cause a validation to occur just to test that one has been defined.

An alternative test implementation could mock the validates_presence_of call and load the file. If the validates_presence_of is defined correctly the class methods will be invoked.
class PhoneNumberTest < Test::Unit::TestCase
Column = ActiveRecord::ConnectionAdapters::Column

test "validates_presence_of is defined for digits" do
PhoneNumber.expects(:validates_presence_of).with(:digits)
load "#{RAILS_ROOT}/app/models/phone_number.rb"
end

end
This method of testing doesn't require actually creating a PhoneNumber instance, instead it tests that the PhoneNumber validations are defined correctly.

The load call is a bit messy; however, we can clean that up by adding another method to ActiveRecord::Base.
class << ActiveRecord::Base
def standard_path
File.expand_path("#{RAILS_ROOT}/app/models/#{self.name.underscore}.rb")
end
end
Following the above code change the test can be rewritten to the code shown below.
class PhoneNumberTest < Test::Unit::TestCase
Column = ActiveRecord::ConnectionAdapters::Column

test "validates_presence_of is defined for digits" do
PhoneNumber.expects(:validates_presence_of).with(:digits)
load PhoneNumber.standard_path
end

end
Which implementation is better? Both implementations provide pros and cons. I suggest trying both out to determine which works best for you (or your team).

9 comments:

  1. I'd be inclined to avoid the second one completely - you should be testing behaviour not implementation details.

    ReplyDelete
  2. A problem with the first example is that you are testing the behavior of the validation; however, you didn't implement the behavior of the implementation. In essence, you are testing that the framework correctly built the validation for the field you specified. And, I try to avoid testing code I'm not responsible for (i.e. frameworks) as often as possible.

    That said, testing the implementation isn't a great plan either. But, is it the best of bad options?

    ReplyDelete
  3. If I write a test for say, "User must have an email address" I might write a test like:

    http://pastie.caboo.se/30056

    (actually I'd write it in a more BDD-oriented style with RSpec but thats besides the point).

    I'm expressing a requirement that the user should be invalid when it doesn't have an email address. This test doesn't test implementation, nor does it test framework functionality. It simply expresses a business requirement without caring how that test is made to pass. And of course we could implement that using our own home-grown solution but following the mantra of "do the simplest thing that works", with Rails, this means using a validation macro.

    ReplyDelete
  4. luke: The problem with the statement "testing behaviour not implementation details" is that it is absolutely correct, in isolation. However, when you start adding variables to the context the lines aren't nearly as distinct between right and wrong.

    Here's a few thoughts along those lines
    -Do you write get and set tests for accessors to test their behavior?
    -Testing a model correctly validates an email address requires a round trip to the database to get the email address column, or you can mock the columns, but that requires some set up. So, is it worth the db connection or the additional set up?
    -Also, what behavior do you test? A validation doesn't just set the behavior of the errors collection, it also sets the behavior of the valid? method. Assuming you have multiple validations, in order for you to test that the valid? method is returning correctly for each error you will need to set up valid data for every validation except the one that you are testing.
    -Do you actually need a test that tests behavior that is not yours? Yes, you could write the validation yourself, but when would you ever do that for a presence of validation? So, are you writing a test for a situation that you would never encounter? And, since you likely are, is it worth the trip to the db and the previously mentioned set up.
    -The 'test behaviour not implementation' mantra was born in languages where declaritive programming is not possible. Are you willing to blindly apply to declaritive programming also?
    -Would you prefer to test all the permutations of valid and invalid situations or write one test and count on the framework to correctly implement the validation.

    There are more examples, but I think that list is enough to show that we are far from a clear answer on this topic, at this point.

    ReplyDelete
  5. Very interesting. I've seen (read 'written') some pretty hairy validation specs that attempt to cover all the variations. The second approach would really clean them up.

    I see Luke's point. It does feel "implementy". But I think you can make a fair argument that testing that the validation gets set by mocking the interface to AR is exactly what we would do if the interface to AR looked a little different.

    Imagine if AR provided some sort of config class. Then you might do something like this:

    specify "should tell config to require email" do
    @config.should_receive(:required_fields).with(@model_class, :email)
    end

    Seems completely natural given that design.

    Thanks Jay - a real thought-provoker.

    ReplyDelete
  6. The problem with testing the implementation (like expecting some validation declaration method to be called), IMHO, that it works only in simple cases. If you have some conditional validation (like "require this field not to be empty only if some other field has particular value") you will need to fine tune the expected arguments for validation declaration call or otherwise you will be testing some other validation, not the one you actually wanted to test. So, my opinion is to have all those complex scenarios explicitly tested instead of relying on assumption, that having certain validation setup will cover all of them.

    Then, you are bound to certain validation implementation. If you would change validation implementation later (e.g. for efficiency reason) and drop validates_* methods in favor of custom validate method you would require rewriting tests.

    Then, you say that testing valid? method require model to be valid. I tend to use following pattern:

    class UserTest < Test::Unit::TestCase
    test 'test data should be valid' do
    assert_valid new_user
    end

    test 'should require name' do
    user = new_user :name => nil
    assert_attribute_invalid user, :name
    end

    test 'should require email' do
    user = new_user :email => nil
    assert_attribute_invalid user, :email
    end

    protected

    def new_user(params = {})
    User.new({ :name => 'Jay', :email => 'jay@example.com' }.merge(params))
    end
    end


    So, here I have test to ensure that helper method creates valid user object that I could use as a base for my validation tests.

    Also, to follow 'one assertion per test' principle, I've added an assert_attribute_invalid method which actually does two tests behind the scene:

    def assert_attribute_invalid object, attribute
    assert ! object.valid?
    assert object.errors.on(attribute)
    end

    ReplyDelete
  7. I also think I prefer the first approach that tests behaviour. There are advantages to testing framework functionality as well. It makes sure that the framework behaves as you expect and that it continues to do so after future upgrades.

    ReplyDelete
  8. You're not really testing the framework in these cases. You're testing that you're calling the framework in the correct way to accomplish what you intend to do.

    The problem with the 2nd example is that you're assuming in the test that you know what the correct framework call is to accomplish what you intend to do. When I've tried mocking calls for similar tests, my first assumption has almost always been incorrect. The other problem is that your test says how you MUST write the code, when there may be other (better?) ways to accomplish the same thing.

    ReplyDelete
  9. > -Would you prefer to test all the permutations of valid and invalid situations or write one test and count on the framework to correctly implement the validation.

    I think this point is interesting. Certainly we shouldn't be testing that the framework's validation mechanism works, but surely it's key here that the same validation mechanism works on external input (the attributes being passed), so to test this class properly we should be sure that, given the inputs we're likely to encounter, the validation enables our class to respond as expected.

    Given the email address example, perhaps it's more useful to consider validating its form. Simply testing:

    Person.expects(:validates_format_of).with(:email_address, :with => EMAIL_REGEXP)

    doesn't strike me as a good test, because we've more than likely just reused the same EMAIL_REGEXP as we wrote in the implementation. Surely this is a case where we'd want to supply a set of malformed strings and check that the validation (i.e. the regular expression that isn't part of the framework) we've declared actually behaves how we'd hoped?

    I suppose putting it succinctly, it does seem less compelling to add many tests for very simple declarative validations, but I think that was more a reflection of the simple example than a general rule.

    ReplyDelete

Note: Only a member of this blog may post a comment.