Ruby Forum RSpec > Help needed

Hi All,

I am new to rspec and it seems that I don't understand some basics.
I need to have a XML import which should parse through XML data
and saves all that in various mysql tables. The XML part works just
fine and I can test this with rspec. However when I try to execute

it "should find country object for DE" do

I get an error. @user.country is a one-to-many relation in the user
table. It seems easy but I don't get it (hmmmm feels like I am still a
newbie).

The error I get is:

'XmlImport should find country object for DE' FAILED
expected #<Country:0x..fdb71beba @name="Country_1001">, got nil (using
.equal?)


Any help very appreciated.
Thanks
Jens

--- model ---

require 'hpricot'
class XmlImport #< ActiveRecord::Base

   attr_reader :doc, :user

   def parse_xml(file)
     @doc = Hpricot.XML(open(file))

     (@doc/:member).each do |data|
       retrieve_and_save_user(data)
     end
   end

   def retrieve_and_save_user(data)
     # email address must be unique for members
     @user = User.find_or_initialize_by_email(email)

     # save user if not existing
     if @user.new_record?
       @user.country =
Country.find_by_short((data/:countryShort).inner_html)
     end
   end
end


---- spec -------

module XmlImportSpecHelper

   def mock_xml_import
     xml_file = RAILS_ROOT + "/spec/fixtures/import-member.xml"
     xml = File.read(xml_file)

     @xml_import = XmlImport.new
     @xml_import.should_receive(:open).exactly(1).times.
       with("any-file-name.xml").
       and_return(xml)
   end

end

describe XmlImport do

   include XmlImportSpecHelper

   before(:each) do
     mock_xml_import
     @xml_import.parse_xml("any-file-name.xml")

     @country = mock_model(Country)
     Country.stub!(:find_by_short).and_return(@country)
   end

   it "should find country object for DE" do
     Country.should_receive(:find_by_short).with("DE").and_return(@country)
     @xml_import.user.country.should equal(@country)
   end
end

Hi Jens,

On 17.5.2008, at 20.34, Jens Carroll wrote:

> table. It seems easy but I don't get it (hmmmm feels like I am still  
> Thanks
>    @doc = Hpricot.XML(open(file))
>    # save user if not existing
> module XmlImportSpecHelper
>
>    @country = mock_model(Country)
>    Country.stub!(:find_by_short).and_return(@country)

In order to have the above stubbing and mocking work, it must be done
*before* you use them in the actual code. It seems to me that
parse_xml is where you want to use them.

>
>  end
>
>  it "should find country object for DE" do
>     
> Country.should_receive(:find_by_short).with("DE").and_return(@country)

Same here. should_receive must always be defined before you run the
code that is expected to do something. In this case, parse_xml and
thus Country.find_by_short is run without any stubbing so it will go
to the database and probably find nothing.

>
>    @xml_import.user.country.should equal(@country)
>  end
> end
> _______________________________________________
> rspec-users mailing list
> rspec-users@rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users

--
Jarkko Laine
http://jlaine.net
http://dotherightthing.com
http://www.railsecommerce.com
http://odesign.fi

On 17 May 2008, at 18:34, Jens Carroll wrote:
> Hi All,
>
> I am new to rspec and it seems that I don't understand some basics.

Hi Jens,

Jarkko answered your real question but there are other things worth
pointing out, more about style than actually making the specs work.


>      with("any-file-name.xml").
>      and_return(xml)
>  end
>
> end

You shouldn't have (hoho) a should_receive here.  You're using the
mock_xml_import in the before step, which is intended to set up mocks
and stubs shared across the examples ("it" blocks).  The code
"@xml_import.should_receive(:open)..." should have its own example.

Also, since you're creating a *real* XmlImport, it doesn't make sense
to call it "mock_xml_import", maybe "new_xml_import" would be clearer.


>  end
>
>  it "should find country object for DE" do
>     
> Country.should_receive(:find_by_short).with("DE").and_return(@country)
>    @xml_import.user.country.should equal(@country)
>  end
> end

Be clear what you are specifying here.  Your call to #parse_xml is in
the before block, which means you can't set expectations on it.  I
like to nest my methods in their own inner describe blocks, eg:

   describe XmlImport do
     before(:each) do
       @xml_import = XmlImport.new
     end

     describe "#parse_xml" do
       it "should open the XML file" do
         # ...
       end

       it "should find country object for DE" do
         # ...
       end
     end
   end

Be aware also that while "xml_import.should_receive(:open)" may work,
it's a bit of a quirk of Ruby.  The real method is define on the
Kernel module, which is included into Object (so you get access to it
everywhere).  Since you know you're dealing with files, I'd use
File.open, and specify that File.should_receive(:open) instead.
Otherwise you may start thinking it's ok to specify interpendencies
between methods in the class being specified.

Finally, this line:

>   @xml_import.user.country.should equal(@country)


is known as a trainwreck- that is, multiple method calls strung
together.  It's usually a sign that something is modelled wrong.  In
this case, it's because your XmlParser class is doing too much.

This section

>   (@doc/:member).each do |data|
>     retrieve_and_save_user(data)
>   end


Is the culprit.  The XmlParser should parse XML, not retrieve and save
user data (I know this, because it's called XmlParser, not
UserDataRetrieverAndSaver, basically.)  A more OO approach would be to
make User responsible for this:

   (@doc/:member).each do |data|
     User.update_from_xml(data)
   end

That way, the spec for User#update_from_xml would have one less level
of indirection, ie just

   @user.country.should equal(@country)

Hope this is useful,
Ashley


--
http://www.patchspace.co.uk/
http://aviewfromafar.net/

Ashley Moran schrieb:
>
>>      with("any-file-name.xml").
> Also, since you're creating a *real* XmlImport, it doesn't make sense 
>>
>
>       it "should open the XML file" do
> it's a bit of a quirk of Ruby.  The real method is define on the 
>
>
> of indirection, ie just
>
>   @user.country.should equal(@country)
>
> Hope this is useful,
> Ashley
>
>
Hi Ashley,

Your hints are most appreciated. Wow looks like I have to change quite a
bit.

I was not aware that "@xml_import.user.country.should equal(@country)"
is already
a trainwreck. I think I have even longer ones - refactoring might be the
magic
word for my code now.

Every hint was really useful for me.

Thanks a million for taking the time to analyze my code.

Have a nice day
Jens

Jarkko Laine schrieb:
>>
>> (using .equal?)
>>
>>  def retrieve_and_save_user(data)
>>
>>    @xml_import.should_receive(:open).exactly(1).times.
>>  before(:each) do
>>
>
> Jarkko Laine
> rspec-users@rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users

Hi Jarkko,

Thanks a lot. The next time when I get a nil object I'd better ask
myself if the mocking and stubbing
has been before the actual code. The real problem was that my whole
rspec test got quite big. I need to break it down in smaller pieces not
to lose the overview.

Have a nice day
Jens

On 18 May 2008, at 18:28, Jens Carroll wrote:

> I was not aware that "@xml_import.user.country.should  
> equal(@country)" is already
> a trainwreck. I think I have even longer ones - refactoring might be  
> the magic
> word for my code now.

Well I guess it's more the wrong sort of leaves on the line :)  It's
not so much the length of the line that's the problem, that's just a
code smell.  The real issue is that it makes more sense for the user
(domain model) to update itself off the XML, rather than have an XML
parser (utility class) go poking around inside the user.

Having had a second look at it, I'd be inclined to say that
XmlImport#parse_xml should really be a class method of User (something
like User.import_all_from_xml).  The only bit of this method that
isn't domain logic is the line

   @doc = Hpricot.XML(open(file))

which should probably be in the controller, or wherever this code gets
called.  You don't want the domain logic depending on filesystem
technicalities and more than the utility code fiddling with the model.

Ashley


--
http://www.patchspace.co.uk/
http://aviewfromafar.net/