[rspec-users] Help needed

Jens Carroll lists at carroll.de
Sun May 18 13:28:25 EDT 2008


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



More information about the rspec-users mailing list