[rspec-users] help with test design

David Chelimsky dchelimsky at gmail.com
Fri Jun 13 12:10:23 EDT 2008


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



More information about the rspec-users mailing list