rspec users - Jun 2008 - help with test design

I''m having trouble figuring out how to drive the design of a class.  
I''m hoping I can get a hint or two from the more experienced BDDers on
this list.

I''m writing an adapter class that sits between a 3rd party library and
my business logic. I would like to extract some information from  
objects available from the 3rd party lib and covert them into ruby  
objects. My desire is to insulate my ruby business logic from changes  
in the 3rd party library by putting all of this conversion code into a  
single adapter class (and some related "event" classes).

Using BDD techniques I have created a pretty simple interface  to the  
adapter class. So far the adapter class has two main public methods  
(plus a bunch of accessors to allow the dependency injection).

#discover_instruments
- this method calls into the 3rd party library and pulls out the  
3rdPartyInstrument object and stores it off for further manipulation

#save_components
- this method permanently saves the 3rdPartyInstrument if it is a  
FUTURE, or if it is a COMBINATION it iterates through each  
sub-3rdPartyInstrument and saves it off

Using mocks and ruby''s brain-dead simple support for dependency  
injection I have been able to mock out and verify this behavior.

Now I need to add another public method which I will call  
#generate_internal_instruments. What I would like this method to do is  
iterate through my #components and create two related objects. Object  
one will be my RubyInstrument class (called something else, but this  
will suffice) and a second is a RubyInstrumentCreationEvent. The  
intention is to pass the 3rdParty component to  
RubyInstrumentCreationEvent.new(component) where it will extract all  
salient details from the 3rdParty component (again, this is for  
insulating the business logic from knowing this 3rd Party lib). Then I  
will pass this RubyInstrumentCreationEvent to RubyInstrument.new (or  
RubyInstrument.build) where it will finish the construction of itself.

Ideally, all of these steps would be private to the Adapter class and  
would expose a single public interface called #build. All of these  
public methods exist solely so I can test these specific behaviors.

Any suggestions on how to accomplish my goals? Is my difficulty  
pointing at a code smell that I am not detecting?

cr

On Wed, Jun 11, 2008 at 10:06 AM, Chuck Remes <cremes.devlist at mac.com>
wrote:
> Ideally, all of these steps would be private to the Adapter class and would
> expose a single public interface called #build. All of these public methods
> exist solely so I can test these specific behaviors.
>
> Any suggestions on how to accomplish my goals? Is my difficulty pointing at
> a code smell that I am not detecting?
I feel a bit brain-dead today, so take this with a grain of salt...but
after reading through your email several times, I''m not sure what your
goals are, what you''ve currently tried, and what particular problems
you''re experiencing.

Pat

On Jun 11, 2008, at 12:06 PM, Chuck Remes wrote:
> I''m having trouble figuring out how to drive the design of a
class.
> I''m hoping I can get a hint or two from the more experienced
BDDers
> on this list.
>
> I''m writing an adapter class that sits between a 3rd party library
> and my business logic. I would like to extract some information from  
> objects available from the 3rd party lib and covert them into ruby  
> objects. My desire is to insulate my ruby business logic from  
> changes in the 3rd party library by putting all of this conversion  
> code into a single adapter class (and some related "event"
classes).
>
> Using BDD techniques I have created a pretty simple interface  to  
> the adapter class. So far the adapter class has two main public  
> methods (plus a bunch of accessors to allow the dependency injection).
>
> #discover_instruments
> - this method calls into the 3rd party library and pulls out the  
> 3rdPartyInstrument object and stores it off for further manipulation
>
> #save_components
> - this method permanently saves the 3rdPartyInstrument if it is a  
> FUTURE, or if it is a COMBINATION it iterates through each  
> sub-3rdPartyInstrument and saves it off
>
> Using mocks and ruby''s brain-dead simple support for dependency  
> injection I have been able to mock out and verify this behavior.
>
> Now I need to add another public method which I will call  
> #generate_internal_instruments. What I would like this method to do  
> is iterate through my #components and create two related objects.  
> Object one will be my RubyInstrument class (called something else,  
> but this will suffice) and a second is a  
> RubyInstrumentCreationEvent. The intention is to pass the 3rdParty  
> component to RubyInstrumentCreationEvent.new(component) where it  
> will extract all salient details from the 3rdParty component (again,  
> this is for insulating the business logic from knowing this 3rd  
> Party lib). Then I will pass this RubyInstrumentCreationEvent to  
> RubyInstrument.new (or RubyInstrument.build) where it will finish  
> the construction of itself.
>
> Ideally, all of these steps would be private to the Adapter class  
> and would expose a single public interface called #build. All of  
> these public methods exist solely so I can test these specific  
> behaviors.
>
> Any suggestions on how to accomplish my goals? Is my difficulty  
> pointing at a code smell that I am not detecting?
Hey Chuck - sounds like you''re trying to plan the whole process out up
front. TDD is about discovery. Here''s what I''d recommend:

Start w/ a simple example of what you want the adapter to do - the  
simplest think you can think of - when you send it the  
#generate_internal_instruments message. Get that to pass. Now the next  
simplest thing. Get that to pass. Refactor. Rinse. Repeat.

As you do this, the method might start to do too much and you''ll see  
opportunities to break it up into smaller private methods or perhaps  
even methods on a brand new object you don''t know about yet.
That''s
all good. Let the design emerge.

If/when you do choose to move things out to other classes, you may  
want to introduce new mocks in the examples and move some of the  
examples to the group describing the new object. That''s all fine.

Don''t anticipate code smells, but keep sniffing. When you *do* smell  
them, deal with them immediately. Don''t wait.

HTH,
David

Forwarded message from Chuck Remes:

Begin forwarded message:
>> From: Chuck Remes <cremes.devlist at mac.com>
>> Date: June 13, 2008 10:31:12 AM CDT
>> To: rspec-users <rspec-users at rubyforge.org>
>> Subject: Re: [rspec-users] help with test design
>>
>>
>> On Jun 11, 2008, at 2:25 PM, David Chelimsky wrote:
>>
>>> On Jun 11, 2008, at 12:06 PM, Chuck Remes wrote:
>>>
>>>> Ideally, all of these steps would be private to the Adapter
class
>>>> and would expose a single public interface called #build. All
of
>>>> these public methods exist solely so I can test these specific
>>>> behaviors.
>>>>
>>>> Any suggestions on how to accomplish my goals? Is my difficulty
>>>> pointing at a code smell that I am not detecting?
>>>
>>> Hey Chuck - sounds like you''re trying to plan the whole
process
>>> out up front. TDD is about discovery. Here''s what
I''d recommend:
>>>
>>> Start w/ a simple example of what you want the adapter to do - the
>>> simplest think you can think of - when you send it the  
>>> #generate_internal_instruments message. Get that to pass. Now the  
>>> next simplest thing. Get that to pass. Refactor. Rinse. Repeat.
>>>
>>
>> [I apologize if this shows up multiple times. I sent it last night  
>> and again this morning but it never showed up on the list or in the  
>> archives. I subsequently unsubscribed and resubscribed. <crossing  
>> fingers 3rd time is the charm>]
>>
>>
>> Warning! Long post ahead! It responds to both Pat & David.
>>
>> Pat, no problem. Let me take another stab at this. I''ll break
>> things out into their own sections. I''ve actually made some  
>> progress since I sent the email so I''ll incorporate those
changes
>> in my thinking.
>>
>> Oh, and to answer David''s response that I am doing the whole  
>> process up front, the code I have so far is a direct result of  
>> tests. I *did* think through the problem beforehand a little bit  
>> because I had to pick where to start.
>>
>> Lastly, I have put the code for the class at the end of this email.  
>> The tests are sprinkled throughout but the class code is in one spot.
>>
>> GOALS
>>
>> 1. Practice BDD so no business-logic code is written without a test  
>> being written first.
>>
>> 2. Create an Adapter class to act as a proxy/adapter between a 3rd  
>> Party Library and my business logic
>>
>> 2a. Adapter class may use additional helper classes to convert the  
>> data inside objects from the 3rd Party lib into ruby objects used  
>> by my business logic
>>
>> 3. End up with well-tested code that only exposes as many public  
>> methods as necessary to accomplish the aforementioned goals
>>
>> CODE GOALS
>>
>> 1. To insulate my business logic from this 3rd party lib, I need to  
>> load and copy a bunch of things. I''m focusing on only one at a
time
>> so each "thing" will potentially get its own class unless I
can
>> refactor the load behavior out to a module or something else.
>>
>> 2. I need to do the following sequence of steps to load and copy  
>> the data.
>> 	a. Get the Orc::Instruments from the 3rd party library
>> 	b. Save the Orc::Instruments
>> 	c. Build my internal shadow copy of the Orc::Instrument which I am  
>> calling Instrument in the ruby namespace
>> 	d. Subscribe to market data updates for each Orc::Instrument
>> 	e. Push all market data updates from Orc::Instrument to Instrument
>>
>>
>> MY EFFORTS SO FAR
>>
>> I created a class called LiquidatorInstrumentAdapter. I skipped the  
>> tests to verify that it exists and other basics. The first test I  
>> wrote was:
>>
>> describe LiquidatorInstrumentAdapter, "discover_instruments"
do
>> before(:each) do
>> @ia = LiquidatorInstrumentAdapter.new
>> @orc = mock("orc")
>> end
>>
>> it "should accept an orc strategy object" do
>> @ia.orc_strategy.should be_nil
>> @ia.orc_strategy = @orc
>> @ia.orc_strategy.should_not be_nil
>> end
>> end
>>
>> In order to fetch the Orc::Instruments, I need to get a reference  
>> to them via a handle I call #orc_strategy. I created an accessor on  
>> the class so I could set the #orc_strategy field. Test went from  
>> failing to passing. I could have also passed this in the  
>> constructor but I prefer the freedom of creating empty objects and  
>> filling them out after the fact. Also, I could have skipped this  
>> test altogether since I''m really just testing that
#attr_accessor
>> works. So what... it helped prime the pump.
>>
>> Now that I have my reference to the orc strategy, I can get the  
>> Orc::Instruments loaded up. So I wrote this test (I created a new  
>> mock in my before block too):
>>
>> it "should get all instruments from Orc" do
>> @orc_instruments.should_receive(:nil?).and_return(false)
>> @orc_instruments 
>> .should_receive(:getKind).and_return(Orc::InstrumentKind::FUTURE)
>> @orc 
>> .should_receive 
>> (:getInstrumentParameter 
>> ).with("instrument").and_return(@orc_instruments)
>> @ia.orc_strategy = @orc
>> @ia.discover_instruments
>> end
>>
>> The logic here is that I should be able to call  
>> #orc_strategy.getInstrumentParameter and get an Orc::Instrument  
>> object for my efforts. I also make sure to do a sanity check for a  
>> nil instrument. Please note that in order for me to inject this  
>> orc_instrument mock that I need to expose another accessor (called  
>> #orc_instruments). This is one of the details that upsets me about  
>> testing, but I''ll get to that at the end.
>>
>> This nil check lead me to my next test.
>>
>> it "should raise an exception if the instrument is nil" do
>> @orc.should_receive(:getInstrumentParameter).and_return(nil)
>> @ia.orc_strategy = @orc
>> lambda { @ia.discover_instruments }.should raise_error
>> end
>>
>> No surprises here; sometimes the 3rd party lib fails to return a  
>> valid object so I need to check for that. To save space, I''ll
skip
>> the last test I wrote which was verifying that the Orc::Instrument  
>> it receives back is of type COMBINATION or FUTURE since those are  
>> the only instrument kinds I want to handle right now. I moved on to  
>> the next step in my logical sequence which was to save these  
>> Orc::Instruments for future reference. So I wrote another test:
>>
>> describe LiquidatorInstrumentAdapter, "save_components" do
>> # skipping the #before block which sets up some mocks
>> it "should save the component when the instrument is a
FUTURE" do
>> @ia 
>> .orc_instrument 
>> .should_receive(:getKind).and_return(Orc::InstrumentKind::FUTURE)
>> @ia.orc_instrument.should_receive(:getInstrument).and_return(1)
>>
@ia.orc_strategy.should_receive(:setParameter).with("components",
>> @ia.orc_components)
>> @ia.orc_components.should be_empty
>> @ia.save_components
>> @ia.orc_components.length.should == 1
>> end
>> end
>>
>> This test verifies that the Orc::Instrument is the proper kind and  
>> that my internal array called #orc_components is empty at the  
>> beginning and contains one element after sending the message. Note  
>> again that I had to expose what should likely be a private  
>> structure in order to test it (attr_accessor :orc_components). I  
>> wrote a few more tests to make sure it properly saves COMBINATIONS  
>> (which contain multiple FUTURES) and that they are saved off  
>> correctly. I also wrote a test to verify an exception is raised if  
>> the instrument is not a COMBINATION a FUTURE (this is probably  
>> unnecessary since it was validated in the prior method).
>>
>> At this point I have a class starting to take shape. Moving on to  
>> the next step takes me to building my shadow copy of these  
>> Orc::Instrument objects as ruby objects. I didn''t know what
this
>> interface should look like so I started out by using mocks and  
>> letting it lead me. After a little experimentation it was clear  
>> that I needed to break this functionality out to its own class  
>> (lots of copying fields from one object to another). I elected to  
>> call it OrcInstrumentCreationEvent. Here''s the test I wrote
for it
>> prior to actually creating that class for real (all mocks).
>>
>> describe LiquidatorInstrumentAdapter,  
>> "generate_instrument_creation_events" do
>> before(:each) do
>> @ia = LiquidatorInstrumentAdapter.new
>> end
>>
>> it "should create a InstrumentCreationEvent for each
component" do
>> orc_components = mock("components")
>> event = mock("instrument event class")
>> event 
>> .should_receive 
>> (:new).exactly(:twice).with(orc_components).and_return(event)
>> orc_components 
>> .should_receive 
>> (:each).and_yield(orc_components).and_yield(orc_components)
>>
>> @ia.orc_components = orc_components
>> ary = @ia.generate_instrument_creation_events(event)
>> ary.size.should == 2
>> end
>> end
>>
>> So this verifies that I create a CreationEvent for each  
>> Orc::Instrument that I saved earlier. All the gory details of  
>> copying the right fields from the Orc::Instrument will be  
>> encapsulated by that class. Note that I need to pass a Class as a  
>> parameter to #generate_instrument_creation_events so that my test  
>> can pass it a mock and verify the behavior. In the real world
I''ll
>> pass a ruby Instrument but ideally (in my mind) this wouldn''t
be a
>> parameter at all. It looks like I am contorting my code so that it  
>> is testable.
>>
>> I still haven''t created my shadow ruby Instrument object yet.
>> Hmmm... let''s write a test.
>>
>> describe LiquidatorInstrumentAdapter,  
>> "generate_internal_instruments" do
>> before(:each) do
>> @ia = LiquidatorInstrumentAdapter.new
>> end
>>
>> it "should create a new Instrument for each event and #build
it" do
>> event_ary = mock("instrument creation event array")
>> instrument_class = mock("instrument class")
>> empty_mock = mock("placeholder")
>> event_ary.should_receive(:each).and_yield(empty_mock)
>> instrument_class.should_receive(:new).and_return(instrument_class)  
>> # reuse this mock
>> instrument_class.should_receive(:build).with(empty_mock)
>> @ia.generate_internal_instruments(event_ary, instrument_class)
>> end
>> end
>>
>> Taking my CreationEvents, I can pass these to the Instrument#build  
>> method and it will know which accessors to use to pull the  
>> appropriate data out. These are all mocks. I''ll create the
real
>> objects a little bit later (not covered in this email). Now
I''m
>> scratching my head wondering if I know what the hell I''m
doing. To
>> test this I have to pass in both an event array and the class name  
>> of the object I want to instantiate and #build. This is all in the  
>> name of testability so I can inject my mocks into the stream and  
>> verify the behavior. But it looks fugly to me. Ideally the last two  
>> methods I wrote would be private to the class and never exposed to  
>> the outside world. But one of the cardinal rules of testing (from  
>> what I''ve read) is that we should *not* test private methods.
Is
>> this a circumstance where that rule doesn''t apply?
>>
>> Anyway, the class code is relatively short. I have about double the  
>> lines in my spec file to test the class. That doesn''t bother
me too
>> much but there are a few things bugging me which I''ll list.
>>
>> THINGS THAT ARE BUGGING ME
>>
>> 1. To use mocks to verify behavior, I have to create many public  
>> accessors to inject the mocks. These public accessors, in most  
>> cases, would not be part of any public API that I would publish for  
>> another programmer to use. Ideally they would set a few things  
>> (like orc_strategy) and then call #build on the whole object which  
>> would privately call most of the methods I have defined as public.
>>
>> 2. I''m doing lots of mock setup in each test. My rule of thumb
has
>> been to break a method out to another (public) method if I have to  
>> setup more than 3 mocks to test its behavior. I don''t know if
this
>> is pointing to a larger problem (lots of setup) or what, but I  
>> would be grateful for any insight.
>>
>> 3. I think some of my mocks are exposing too much of the
class''
>> implementation. For example, in my OrcInstrumentCreationEvent test  
>> I tell the orc_components mock to expect the message :each which is  
>> exposing how I am iterating. What if I want to use a while or a for  
>> loop? That shouldn''t be any business of the test. It cares
about
>> behavior, not implementation but I don''t see how else to get
the
>> mock to return what I need.
>>
>> I would love to have this critiqued by veteran BDDers. Be as blunt  
>> as necessary... I have broad shoulders.
>>
>> cr
>>
>> CLASS CODE
>>
>> module Orc
>> import com.orcsoftware.liquidator.InstrumentKind
>> end
>>
>> class LiquidatorInstrumentAdapter
>>
>> attr_accessor :orc_strategy
>> attr_accessor :orc_components
>> attr_accessor :orc_instrument
>> attr_accessor :instruments
>>
>> def initialize
>> @orc_components ||= []
>> @instruments ||= []
>> end
>>
>> def build
>> discover_instruments
>> save_components
>> events =  
>> generate_instrument_creation_events 
>> (LiquidatorInstrumentCreationEvent)
>> generate_internal_instruments(events, Instrument)
>> subscribe_to_components
>> end
>>
>> def discover_instruments
>> @orc_instrument =
orc_strategy.getInstrumentParameter("instrument")
>> raise TypeError, "Instrument parameter was nil!", caller[0]
if
>> orc_instrument.nil?
>> kind = orc_instrument.getKind
>> if (kind != Orc::InstrumentKind::COMBINATION && kind !=  
>> Orc::InstrumentKind::FUTURE)
>>   raise TypeError, "Instrument parameter was the wrong type  
>> [#{kind}], should be either [#{Orc::InstrumentKind::COMBINATION}]  
>> or [#{Orc::InstrumentKind::FUTURE}]"
>> end
>> end
>>
>> def save_components
>> case orc_instrument.getKind
>> when Orc::InstrumentKind::COMBINATION
>>   save_combination_components
>> when Orc::InstrumentKind::FUTURE
>>   save_future_component
>> else
>>   raise TypeError, "Instrument is the wrong kind! :  
>> [#{orc_instrument.getKind}]", caller[0]
>> end
>> orc_strategy.setParameter("components", orc_components)
>> end
>>
>> def subscribe_to_components
>> orc_components.each { |component| component.depthSubscribe }
>> end
>>
>> def generate_instrument_creation_events(klass)
>> events = []
>> orc_components.each do |component|
>>   events << klass.new(component)
>> end
>> events
>> end
>>
>> def generate_internal_instruments(event_ary, klass)
>> event_ary.each do |event|
>>   instruments << klass.new
>>   build_strategy_instrument(instruments.last, event)
>> end
>> end
>>
>> private
>>
>> def save_combination_components
>> orc_instrument.getComponents.each do |component|
>>   orc_components << component.getInstrument
>> end
>> end
>>
>> def save_future_component
>> orc_components << orc_instrument.getInstrument
>> end
>>
>> def build_strategy_instrument(instrument, creation_event)
>> instrument.build(creation_event)
>> end
>> end
>>
>