September 1, 2007

rails - save! broke the contract - mocking to rescue

Posted by Sudhindra Rao

Detail descriptions of mockist and classicist are on Martin Fowler's blog.


Consider

   class Person < ActiveRecord::Base
       #has name, address, home, business, cell phone numbers
      .... more object behaviour
   end


The intended behaviour is to do some system specific tasks incognito - lets say send an email to all his friends.

So we alias the save method as follows

   alias :save_with_notification, :save


   def save_with_notifications
      send_emails
      save
   end

This works well for save. But when somewhere we call save! it all breaks - i.e. our save_with_notifications is not called and nobody gets notified.

The root cause(as analysed by David Vollbracht (not this one)) of this problem was this piece of rails code

   def save
      create_or_update
   end

   # Attempts to save the record, but instead of just returning

   # false if it couldn't happen, it raises a

   # RecordNotSaved exception

   def save!

      create_or_update || raise(RecordNotSaved)

   end


save! does not call save.

On further investigation I found functional tests around the base class for active_record.

But what was missed was a good unit test with mocking like so

   test "save! should successfully save object by calling save" do
      base = Base.new
      base.expects(:save).returns(true)
      assert_nothing_raised base.save!
   end

   test "save! should throw on exception if save fails" do
      base = Base.new
      base.expects(:save).returns(false)
      assert_raises RecordNotSaved, base.save!
   end

Here is a place where mocking does a much better job at testing a specific contract that is a convention and was missed due to black box testing.

This is not the only way to fix this problem though.

The real problem is that the requirements did not capture the fact of aliasing save or save! methods.

Testing this scenario functionally is perfectly possible though the testing could get more involving.

The tests in that case would make sure that aliasing of save method does not break save!.

But the above mocked tests are as simple as it gets and they reflect the simpler(obvious?) underlying requirement of save! calling save.

Note: Formatting code on blogger sucks

4 comments:

Dan Manges said...

I disagree - classicists could have written a test that notifications were sent when calling save!

xmlblog said...

Why not use Active Record observers and let them worry about the notification mechanics? You can also test these cleanly.

Sudhindra Rao said...

Dan,
I did not represent the solution cleanly enough. This is something a classic test could look like. But looks like a lot more work than the mocked test(which has clearer intent).

test "save! should successfully save object by calling aliased save" do
manager1 = Person.new(:name => "Manager")
manager1.save!
person = Person.new(:name => "Kramer Cosmo")
person.save!
assert_equal 1, manager.emails.size
assert_equal "Name changed to Kramer Cosmo", manager.emails.first
end

Sudhindra Rao said...

xmlblog,
I am sure that activerecord observers are a better way to send notifications and I would use them if I were sending notifications or doing something on state change.
In this case sending email was just an example. The real situation could be - save associated objects with this object, create associated objects or set some parameters on other objects or this one.

The focus of this article was of aliasing save and then not seeing it work on save!(breaking the contract of the public interface that is.)
I updated the title to clarify this.

Welcome