thr3ads.net - rspec users - [rspec-users] Help needed [May 2008]

If this information is useful, please help other people find it:
Share via:

Jens Carroll

2008-May-17 17:34 UTC

[rspec-users] 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

Jarkko Laine

2008-May-17 18:21 UTC

head link

[rspec-users] Help needed

Hi Jens,

On 17.5.2008, at 20.34, Jens Carroll wrote:
> 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)
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 at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users
--
Jarkko Laine
http://jlaine.net
http://dotherightthing.com
http://www.railsecommerce.com
http://odesign.fi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 2417 bytes
Desc: not available
URL:
<http://rubyforge.org/pipermail/rspec-users/attachments/20080517/9e7dcf98/attachment.bin>

Ashley Moran

2008-May-18 08:40 UTC

head link

[rspec-users] Help needed

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.

> ---- 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
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.

> 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
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/

Jens Carroll

2008-May-18 17:28 UTC

head link

[rspec-users] Help needed

Ashley Moran schrieb:> 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.
>
>
>> ---- 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
>
> 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.
>
>
>> 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
>
> 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
>
>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

Jens Carroll

2008-May-18 17:29 UTC

head link

[rspec-users] Help needed

Jarkko Laine schrieb:> Hi Jens,
>
> On 17.5.2008, at 20.34, Jens Carroll wrote:
>
>> 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)
>
> 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 at rubyforge.org
>> http://rubyforge.org/mailman/listinfo/rspec-users
>
> -- 
> Jarkko Laine
> http://jlaine.net
> http://dotherightthing.com
> http://www.railsecommerce.com
> http://odesign.fi
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> rspec-users mailing list
> rspec-users at 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

Ashley Moran

2008-May-18 18:23 UTC

head link

[rspec-users] Help needed

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/

Apparently Analagous Threads

Search for more reasonably related threads

rspec users - May 2008 - Help needed

[rspec-users] Help needed

[rspec-users] Help needed

[rspec-users] Help needed

[rspec-users] Help needed

[rspec-users] Help needed

[rspec-users] Help needed

Apparently Analagous Threads