Forum: RSpec Rate my code: refactoring from spec

Announcement (2017-05-07): www.ruby-forum.com is now read-only since I unfortunately do not have the time to support and maintain the forum any more. Please see rubyonrails.org/community and ruby-lang.org/en/community for other Rails- und Ruby-related community platforms.
Fernando P. (Guest)
on 2009-04-15 20:36
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?
Pat M. (Guest)
on 2009-04-15 21:31
(Received via mailing list)
On Wed, Apr 15, 2009 at 9:36 AM, Fernando P. 
<removed_email_address@domain.invalid>
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
Fernando P. (Guest)
on 2009-04-16 11:36
> 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.
Fernando P. (Guest)
on 2009-04-16 12:35
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. (Guest)
on 2009-04-16 13:19
(Received via mailing list)
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 :-)

hth
joaquin
Fernando P. (Guest)
on 2009-04-16 13:58
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?
Fernando P. (Guest)
on 2009-04-16 14:24
> 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/su...

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 topic is locked and can not be replied to.