Rate my code: refactoring from spec

Testing RSpec
1

Hi,

I used to have the following method:

def Paypal.process_ipn(params)
…
paypal = create!(params)
paypal.order.update_from_ipn(completed?)
end

That method obviously is not easily specable because of the double dot
method call, and when specing it, it would hit the DB for nothing. I
used to actually spec the associated order status to know if everything
went on well which breaks the isolation of unit tests. Bad bad bad.

So I refactored it as following:

def Paypal.new_from_ipn(params)
…
new(params)
end

So that it doesn’t hit the DB when I spec the method, and specing the
method can now be done in isolation, as I only set some values returned
by IPN to something more compliant with my DB storage and then in the
controller that calls this method:

paypal = Paypal.new_from_ipn(params)
paypal.update_order if paypal.save!

So the paypal instance still gets saved, but instead of getting saved in
the model it occurs in the controller, no big deal. Also I added a new
instance method to Paypal class to abid to Demeter’s law:

def update_order
order.update_from_paypal_ipn!(completed?)
end

But now I am wondering how to spec this instance method, I thought of
the following:

it “should update the associated order” do
@paypal = Paypal.new(:payment_status => ‘Completed’)
@order = @paypal.order
@order.expects(:update_from_paypal_ipn!).with(true)

@paypal.update_order
end

Is that spec acceptable? I mean I am not sure about the following two
lines:

@order = @paypal.order
@order.expects(:update_from_paypal_ipn!).with(true)

I thought about this other solution which is slightly more complicated:
@order = Order.new
@paypal = Paypal.new(:payment_status => ‘Completed’)
@paypal.stubs(:order).returns(@order)
@order.expects(:update_from_paypal_ipn!).with(true)
@paypal.update_order

So which is best?

And what do you think of my refactoring and my new specs? Did I improve
the code or is it just crap?

2

On Wed, Apr 15, 2009 at 9:36 AM, Fernando P. [email protected]
wrote:

That method obviously is not easily specable because of the double dot
new(params)

lines:

I would probably move the call to update_order into a
before/after_create on Paypal. Then I would write a spec on Paypal
that checks that the right thing happened. I would not likely use a
mock unless Order#update_from_paypal_ipn! is very complex.

describe Paypal, “from IPN” do
it “should update its order when created” do
paypal = Paypal.new_from_ipn :ipn => {:completed => true}
paypal.save!
paypal.order.should be_completed
end
end

class Paypal
after_create :update_order

def update_order
order.update_from_paypal_ipn! completed?
end
end

By doing it in a callback you get

  • transaction semantics for free
  • ability to use resource_controller
  • simpler model API

If you only want to update the order when Paypal.new_from_ipn is
called (and not Paypal.new, for example) then just put an ivar in
Paypal and check for it in the callback.

So to recap, I would test this behavior via the Paypal examples,
because that’s where the behavior originates. I may or may not mock
the call to order.update_from_paypal depending on how complex it is.

Does that make sense?

Pat

3

So to recap, I would test this behavior via the Paypal examples,
because that’s where the behavior originates. I may or may not mock
the call to order.update_from_paypal depending on how complex it is.

Does that make sense?

Argh! I had sent you an answer but for some reason my session expired
and the message didn’t go through.

Anyway, thank you Pat for your message, I like to check out your blog to
fetch some tips on how to spec correctly.

The callback is a good idea, I don’t use them often enough.

order.update_from_paypal is a simple method that just sets the order
state to ‘paid’ or ‘unpaid’ depending on the payment status. I use other
payment gateways so it is paypal agnostic. Now that I test my code I
tend to write smaller methods.

4

By the way in Rails I am now finding myself replacing:
update_attributes, create! and their friends with something that looks
like:

new(…)
save!

Then in the spec I stub the save! method so that it doesn’t hit the DB,
and then I can easily compare the object attributes if they are as
expected.

Pros:

  • specs are lightning fast

Cons:

  • data isn’t actually inserted in DB, so there is a 0.000001% chance
    that the object has bad attributes that would raise an error if it was
    actually saved in DB. But that would mean that my spec is false as I
    myself set the comparison value.

Is it clever or not to do something like that? Maybe I can use that idea
sometimes, and the other times it is safer to really save the object in
DB?

5

Joaquin Rivera P. wrote:

hey there,
maybe you should take a look at solutions that fake your database in
memory
for such cases, saving your time doing all that stubbing and mocking
Yeah you are right. I am refactoring (messing up?) code because I have a
DB constraint, so instead of rewriting the code I should find a better
DB to use during tests.

What’s up will nullDB? I once saw the developper post a few message on
the mailing-list. Anyone using it with success?

6

hey there,
maybe you should take a look at solutions that fake your database in
memory
for such cases, saving your time doing all that stubbing and mocking, I
don’t remember right now but I think I saw some projects for such
approach,
maybe someone knows better than I do :slight_smile:

hth
joaquin

7

What’s up will nullDB? I once saw the developper post a few message on
the mailing-list. Anyone using it with success?

I saw a post by Pat M. and he talked about Sqlite in-memory, so I
decided to give it a try using this tutorial:
http://www.mathewabonyi.com/articles/2006/11/26/superfast-testing-how-to-in-memory-sqlite3/

When I run ‘rake test’, I have 1 failure in one of my test file. if I
test that file alone, there is no failure. The test that fails doesn’t
hit the DB at any time though, so it’s really strange.

So for now I will forget about hitting the database and in-memory
databases, and just focus on writing my specs, but it’s definitely an
interesting idea.