Rate my code: refactoring from spec

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?

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

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.

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?

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?

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

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.

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs