From: Chuck Remes <[EMAIL PROTECTED]>
Date: June 13, 2008 10:31:12 AM CDT
To: rspec-users <[email protected]>
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
