rspec users - Aug 2010 - [RSpec] Testing a helper with or without model layer?

Hi guys,

I wanted to get a more experienced opinion about a test I had to do.

I posted the code at pastie with comments for best visual readability:

http://pastie.org/private/qvvrxubslvia2nv6l3km4q

Any feedback is appreciated

Thanks
-- 
Posted via http://www.ruby-forum.com/.

On Aug 3, 2010, at 6:34 AM, Bruno Cardoso wrote:
> Hi guys,
> 
> I wanted to get a more experienced opinion about a test I had to do.
> 
> I posted the code at pastie with comments for best visual readability:
> 
> http://pastie.org/private/qvvrxubslvia2nv6l3km4q
Even though the code is in a helper, it depends heavily on the model. This
exhibits a code smell called Feature Envy, in which one object (the helper) does
some computation but another object (the CfgInterface model) has all the data.
Based on that, one might argue this method belongs on the model anyhow, in which
case the argument of stubbing out the model layer makes little sense.

There are also several expectations that overlap. It doesn''t matter
that the class is a Hash - what matters is that it behaves like a hash, which is
demonstrated in other expectations. The fact that the keys exist is also
demonstrated in the expectations that access those keys.

Based on those comments, I''d probably do something like
http://gist.github.com/506366. Then I''d figure out how to swap in
factories for the fixtures - right now there''s no way to understand
what ''CONS_ALL_ACCOUNT'' means without looking elsewhere.

HTH,
David

> Any feedback is appreciated
> 
> Thanks
> -- 
> Posted via http://www.ruby-forum.com/.
> _______________________________________________
> rspec-users mailing list
> rspec-users at rubyforge.org
> http://rubyforge.org/mailman/listinfo/rspec-users

Hi David
> Even though the code is in a helper, it depends heavily on the model. 
> This exhibits a code smell called Feature Envy, in which one object (the 
> helper) does some computation but another object (the CfgInterface 
> model) has all the data. Based on that, one might argue this method 
> belongs on the model anyhow, in which case the argument of stubbing out 
> the model layer makes little sense.
I see your point and in this case you are probably right but what if my 
helper was more heavy on the processing and still very data dependent ? 
Should I use fixtures on the helper or should I stub/mock whatever I 
need to avoid the model layer within the helper ?
> There are also several expectations that overlap. It doesn''t
matter that
> the class is a Hash - what matters is that it behaves like a hash, which 
> is demonstrated in other expectations. The fact that the keys exist is 
> also demonstrated in the expectations that access those keys.
Good point, thanks.
> Based on those comments, I''d probably do something like 
> http://gist.github.com/506366. Then I''d figure out how to swap in 
> factories for the fixtures - right now there''s no way to
understand what
> ''CONS_ALL_ACCOUNT'' means without looking elsewhere.
''CONS_ALL_ACCOUNT'' actually has no impact when using
stub/mocks,
''CONS_ALL_ACCOUNT'' was the input parameter the model was using
to fetch
data from the BD, in the second form of the test with stubs/mocks, this 
input parameter has no meaning since I''m controlling all return 
parameters.
-- 
Posted via http://www.ruby-forum.com/.

On Aug 3, 2010, at 9:23 AM, Bruno Cardoso wrote:
> Hi David
> 
>> Even though the code is in a helper, it depends heavily on the model. 
>> This exhibits a code smell called Feature Envy, in which one object
(the
>> helper) does some computation but another object (the CfgInterface 
>> model) has all the data. Based on that, one might argue this method 
>> belongs on the model anyhow, in which case the argument of stubbing out
>> the model layer makes little sense.
> 
> I see your point and in this case you are probably right but what if my 
> helper was more heavy on the processing and still very data dependent ? 
> Should I use fixtures on the helper or should I stub/mock whatever I 
> need to avoid the model layer within the helper ?
It really depends on how deep the helper is reaching into the model. Ideally,
when you''re stubbing a layer, you want to be able to stub one thing. In
this example, there are many things that need to be stubbed. A common example is
a display formatter for a person''s name:

it "concats the first and last name" do
  person = double(''person'', :first_name => "Joe",
:last_name => "Smith")
  helper.full_name_for(person).should == "Joe Smith"
end

Here we''re just stubbing a couple of values on one object - simple. In
this case it makes sense to just stub the model.
> 
>> There are also several expectations that overlap. It doesn''t
matter that
>> the class is a Hash - what matters is that it behaves like a hash,
which
>> is demonstrated in other expectations. The fact that the keys exist is 
>> also demonstrated in the expectations that access those keys.
> 
> Good point, thanks.
> 
>> Based on those comments, I''d probably do something like 
>> http://gist.github.com/506366. Then I''d figure out how to swap
in
>> factories for the fixtures - right now there''s no way to
understand what
>> ''CONS_ALL_ACCOUNT'' means without looking elsewhere.
> 
> ''CONS_ALL_ACCOUNT'' actually has no impact when using
stub/mocks,
> ''CONS_ALL_ACCOUNT'' was the input parameter the model was
using to fetch
> data from the BD, in the second form of the test with stubs/mocks, this 
> input parameter has no meaning since I''m controlling all return 
> parameters.

David Chelimsky wrote:
> It really depends on how deep the helper is reaching into the model. 
> Ideally, when you''re stubbing a layer, you want to be able to stub
one
> thing. In this example, there are many things that need to be stubbed. A 
> common example is a display formatter for a person''s name:
> 
> it "concats the first and last name" do
>   person = double(''person'', :first_name =>
"Joe", :last_name => "Smith")
>   helper.full_name_for(person).should == "Joe Smith"
> end
> 
> Here we''re just stubbing a couple of values on one object -
simple. In
> this case it makes sense to just stub the model.
Thanks again David.
-- 
Posted via http://www.ruby-forum.com/.