Ashley Moran
2007-Feb-20 12:07 UTC
[rspec-users] How to spec code with multiple (interacting) paths
Hi Code with a large number of different paths is probably the biggest problem I have right now. I''ve made a sample class that illustrates the simplest case of one parameter that takes two values: class DataStorer def initialize(logger, emailer, db_updater, do_update_db_step) @logger = logger; @emailer = emailer; @db_updater = db_updater @do_update_db_step = do_update_db_step end def store(data) @logger.log("I was told to store ''#{data}''") @emailer.send_email("Somebody stored ''#{data}''") @db_updater.update_db("SET data = ''#{data}'' WHERE id = 1") if @do_update_db_step end end I need to specify the behaviour of my DataStorer objects when they told to/not to update the database. Here''s a full spec for the case when we do want to update the database: context "A DataStorer where ''do_update_db_step'' is true" do setup do @logger = mock("logger"); @logger.stub!(:log) @emailer = mock("emailer"); @emailer.stub!(:send_email) @db_updater = mock("db_updater"); @db_updater.stub! (:update_db) @data_storer = DataStorer.new(@logger, @emailer, @db_updater, true) end specify "should log the data given when asked to store" do @logger.should_receive(:log).with("I was told to store ''moo''") @data_storer.store("moo") end specify "should email the data given when asked to store" do @emailer.should_receive(:send_email).with("Somebody stored ''moo''") @data_storer.store("moo") end specify "should store the data given to the DB when asked to store" do @db_updater.should_receive(:update_db).with("SET data = ''moo'' WHERE id = 1") @data_storer.store("moo") end end Now the problem is we don''t know what it should do when we tell it to NOT update the database. Here are things I''ve done in the past. First, a separate spec for the differences in behaviour. # Solution 1 - separate spec for differences context "S1 A DataStorer where ''do_update_db_step'' is false" do setup do @logger = mock("logger"); @logger.stub!(:log) @emailer = mock("emailer"); @emailer.stub!(:send_email) @db_updater = mock("db_updater"); @db_updater.stub! (:update_db) @data_storer = DataStorer.new(@logger, @emailer, @db_updater, false) end specify "should NOT store the data given to the DB when asked to store" do @db_updater.should_not_receive(:update_db) @data_storer.store("moo") end end I have a bad feeling about this. If I change the emailer line in the store method to this: @emailer.send_email("Somebody stored ''#{data}''") if @do_update_db_step The behaviour has changed and the specs still pass. This seems bad. Next try, duplicate the entire spec and change the relevant bits: # Solution 2 - copy, paste, update context "A DataStorer where ''do_update_db_step'' is false" do setup do # ... end specify "should log the data given when asked to store" do # ... end specify "should email the data given when asked to store" do # ... end specify "should NOT store the data given to the DB when asked to store" do # ... end end Great for a boolean, not so hot when 6 different things can change - you end up with 7 times the code of the single spec. Probably a very bad idea with two parameters that can each take significant 10 values. This violates DRY. I don''t know if that''s a priority in specs or not. I''m sure code coverage is more important, but it quickly gets out of hand. Next attempt - generate the specs in a loop. I''ve done this before, and the exact structure of the contexts and specs depend on exactly what the parameters do. Some times I''ve been tempted to do this, but the difference in specs are significant enough, and the number of variants small enough, that copy-paste-update seems better. # Solution 3 - dynamic contexts and specs [true, false].each do |do_update_db_step| context "A DataStorer where ''do_update_db_step'' is # {do_update_db_step}" do setup do @logger = mock("logger"); @logger.stub!(:log) @emailer = mock("emailer"); @emailer.stub! (:send_email) @db_updater = mock("db_updater"); @db_updater.stub! (:update_db) @data_storer = DataStorer.new(@logger, @emailer, @db_updater, do_update_db_step) end specify "should log the data given when asked to store" do @logger.should_receive(:log).with("I was told to store ''moo''") @data_storer.store("moo") end specify "should email the data given when asked to store" do @emailer.should_receive(:send_email).with("Somebody stored ''moo''") @data_storer.store("moo") end case do_update_db_step when true specify "should store the data given to the DB when asked to store" do @db_updater.should_receive(:update_db).with("SET data = ''moo'' WHERE id = 1") @data_storer.store("moo") end when false specify "should NOT store the data given to the DB when asked to store" do @db_updater.should_not_receive(:update_db) @data_storer.store("moo") end end end end This works well if you like reading the formatted output (eg in TextMate). Unfortunately it makes the spec code a lot harder to read. I can''t find an ideal solution to this situation, but unfortunately it crops up quite often for me. Has anyone got any experiences or opinions that will help? Cheers Ashley
Jerry West
2007-Feb-20 13:05 UTC
[rspec-users] How to spec code with multiple (interacting) paths
I doubt there is an ideal solution. You might try to encapsulate the different behaviours in different classes (each spec''d separately). Where there are true alternatives, you could use SimpleDelegator to delegate to the appropriate class depending on your flag. No less work but conceptually cleaner perhaps. Alternatively (better?) work to eliminate the flag. especially if it''s a do / don''t do choice. If you don''t need to do something, don''t call the code. Push the decision and hence the spec up a level. The calling routine should not do anything if the flag is false. This is the boundary case of delegation: there''s no need to spec a class that doesn''t do anything. Lastly, you might think of the spec from the point of view of the flag, not the object. In other words, the customer wants something to happen under these circumstances and these other things to happen under those circumstances. Write your code to match these specs and refactor the commonality into shared routines, using your specs to increase your confidence that you haven''t introduced (too many!) bugs. BTW, it''s almost always possible to change code somewhere and still pass the spec - the trick is to write the spec before you change the code! BDD isn''t a guarantee of correctness, merely an aid to design and coding that increases your confidence that your code will do (only) what is required. Best wishes, Jerry> I can''t find an ideal solution to this situation, but unfortunately > it crops up quite often for me. Has anyone got any experiences or > opinions that will help? > > Cheers > Ashley > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > rubyforge.org/mailman/listinfo/rspec-users >
David Chelimsky
2007-Feb-20 16:57 UTC
[rspec-users] How to spec code with multiple (interacting) paths
On 2/20/07, Jerry West <jerry.west at ntlworld.com> wrote:> I doubt there is an ideal solution. You might try to encapsulate the > different behaviours in different classes (each spec''d separately). > Where there are true alternatives, you could use SimpleDelegator to > delegate to the appropriate class depending on your flag. No less work > but conceptually cleaner perhaps. > > Alternatively (better?) work to eliminate the flag. especially if it''s a > do / don''t do choice. If you don''t need to do something, don''t call the > code. Push the decision and hence the spec up a level. The calling > routine should not do anything if the flag is false. This is the > boundary case of delegation: there''s no need to spec a class that > doesn''t do anything. > > Lastly, you might think of the spec from the point of view of the flag, > not the object. In other words, the customer wants something to happen > under these circumstances and these other things to happen under those > circumstances. Write your code to match these specs and refactor the > commonality into shared routines, using your specs to increase your > confidence that you haven''t introduced (too many!) bugs. BTW, it''s > almost always possible to change code somewhere and still pass the spec > - the trick is to write the spec before you change the code!AND watch it fail. That is key!!!!!! The cycle (from TDD) is: 1. write a failing spec 2. make it pass with the simplest thing that could possibly work 3. refactor to eliminate duplication Note that "duplication" here refers to duplication either in the subject code or between the spec and the subject code. IMO, some duplication is OK in specs. As to Ashley''s initial post, I agree with Jerry in that the first thing I''d look to do is see how I could break up some of these responsibilities. FWIW, whenever I find that something is hard to spec/test, I look at that as a design problem. But sometimes the simplest thing that could possibly work isn''t all that simple and you have to find trade-offs between complexity in the system and complexity in the specs. Not too helpful, I realize. Cheers, David> BDD isn''t > a guarantee of correctness, merely an aid to design and coding that > increases your confidence that your code will do (only) what is required. > > Best wishes, > Jerry > > I can''t find an ideal solution to this situation, but unfortunately > > it crops up quite often for me. Has anyone got any experiences or > > opinions that will help? > > > > Cheers > > Ashley > > _______________________________________________ > > rspec-users mailing list > > rspec-users at rubyforge.org > > rubyforge.org/mailman/listinfo/rspec-users > > > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > rubyforge.org/mailman/listinfo/rspec-users >
Ashley Moran
2007-Feb-22 15:40 UTC
[rspec-users] How to spec code with multiple (interacting) paths
On 20 Feb 2007, at 13:05, Jerry West wrote:> I doubt there is an ideal solution. You might try to encapsulate the > different behaviours in different classes (each spec''d separately). > Where there are true alternatives, you could use SimpleDelegator to > delegate to the appropriate class depending on your flag. No less > work > but conceptually cleaner perhaps.Jerry One of the times this crops up most is with Rails controllers that behave differently based on their parameters. So, for example, I''m writing a credit card form now that behaves differently if there is a start date or not. Another one was a case when a form action was receiving 3 models and 3 confirmation checkboxes, and I wanted to spec the behaviour in the case of each being invalid. I''m not sure I could really split these up anyway.> Alternatively (better?) work to eliminate the flag. especially if > it''s a > do / don''t do choice. If you don''t need to do something, don''t > call the > code. Push the decision and hence the spec up a level. The calling > routine should not do anything if the flag is false. This is the > boundary case of delegation: there''s no need to spec a class that > doesn''t do anything.I gave a boolean as a simple example. I''ve wrote other code that does all sorts of wierd stuff depending on where values lie, so there are many possible paths. Again, I''m mainly concerned right now with controllers, so the only layer above is the browser. And if I''m writing models, the last thing I want to do is push decisions up into the controller, because it would divide the business logic. I suppose what I could do with controllers is split out the code that deals with the parameters, something along the lines of (but better thought out than) this brain dump: class ApplicantController < ApplicationController class SaveDetailsParameterParser def new(params) # create models end # models def applicant; end def address; end def vehicle; end def models_valid?; end end def save_details p = SaveDetailsParameterParser.new(params) redirect_to :action => ''error_page'' if !p.models_valid? end end This would be acting like a factory for all objects required by the action, and could be specified independently. Then I wouldn''t need to check that the controller behaves correctly under 3 different invalid models, just that if anything is invalid it behaves correctly. (Pretty much identical to the way you spec the individual validations in a model, then spec that the controller behaves correctly if its invalid.) Maybe you were thinking along these lines? It''s hard to put things across sometimes without writing code out. Ashley
Ashley Moran
2007-Feb-22 15:54 UTC
[rspec-users] How to spec code with multiple (interacting) paths
On 20 Feb 2007, at 16:57, David Chelimsky wrote:> AND watch it fail. That is key!!!!!! The cycle (from TDD) is: > > 1. write a failing specOh I''ve been caught out by writing incorrectly passing specs because I didn''t run them before I wrote the code. It happened today to the guy I''m working with on some RJS code.> 2. make it pass with the simplest thing that could possibly work > 3. refactor to eliminate duplication > > Note that "duplication" here refers to duplication either in the > subject code or between the spec and the subject code. IMO, some > duplication is OK in specs. > > As to Ashley''s initial post, I agree with Jerry in that the first > thing I''d look to do is see how I could break up some of these > responsibilities. FWIW, whenever I find that something is hard to > spec/test, I look at that as a design problem. But sometimes the > simplest thing that could possibly work isn''t all that simple and you > have to find trade-offs between complexity in the system and > complexity in the specs.I suspect I''m getting far too much duplication in specs. I''m forced to generate contexts in a loop, or copy-and-paste whole contexts far too often. I''ll try and modularise some of the application logic in my controllers and see if it sorts itself out. I never thought before about refactoring to eliminate duplication in specs, but it makes sense really. Ashley
David Chelimsky
2007-Feb-22 17:05 UTC
[rspec-users] How to spec code with multiple (interacting) paths
On 2/22/07, Ashley Moran <work at ashleymoran.me.uk> wrote:> > On 20 Feb 2007, at 16:57, David Chelimsky wrote: > > > AND watch it fail. That is key!!!!!! The cycle (from TDD) is: > > > > 1. write a failing spec > > Oh I''ve been caught out by writing incorrectly passing specs because > I didn''t run them before I wrote the code. It happened today to the > guy I''m working with on some RJS code. > > > 2. make it pass with the simplest thing that could possibly work > > 3. refactor to eliminate duplication > > > > Note that "duplication" here refers to duplication either in the > > subject code or between the spec and the subject code. IMO, some > > duplication is OK in specs. > > > > As to Ashley''s initial post, I agree with Jerry in that the first > > thing I''d look to do is see how I could break up some of these > > responsibilities. FWIW, whenever I find that something is hard to > > spec/test, I look at that as a design problem. But sometimes the > > simplest thing that could possibly work isn''t all that simple and you > > have to find trade-offs between complexity in the system and > > complexity in the specs. > > I suspect I''m getting far too much duplication in specs. I''m forced > to generate contexts in a loop, or copy-and-paste whole contexts far > too often. I''ll try and modularise some of the application logic in > my controllers and see if it sorts itself out. > > I never thought before about refactoring to eliminate duplication in > specs, but it makes sense really.My only bit of unsolicited advice w/ this is that you keep reminding yourself that specs are not the same as production code. Eliminating duplication is helpful, but it''s equally (if not more) important that you don''t have to debug your specs (i.e. trace paths through several files, etc). Everything you need to know for a given spec should be right in front of you. Hope that is helpful. Cheers, David> > Ashley > _______________________________________________ > rspec-users mailing list > rspec-users at rubyforge.org > rubyforge.org/mailman/listinfo/rspec-users >
Jerry West
2007-Feb-26 10:50 UTC
[rspec-users] How to spec code with multiple (interacting) paths
> This would be acting like a factory for all objects required by the> action, and could be specified independently. Then I wouldn''t need > to check that the controller behaves correctly under 3 different > invalid models, just that if anything is invalid it behaves > correctly. (Pretty much identical to the way you spec the individual > validations in a model, then spec that the controller behaves > correctly if its invalid.) > > Maybe you were thinking along these lines? It''s hard to put things > across sometimes without writing code out. > > Ashley Yes, I think so; the controller delegates the matter where possible, so your controller spec simply checks that the delegatee is called. The spec for the delegate(e) worries about ensuring that the correct outcomes arising in each of the myriad parameter combinations. I''ve also been puzzling over the true application of "Tell don''t ask", which I haven''t really wrapped my head around yet. To quote... The problem is that, as the caller, you should not be making decisions based on the state of the called object that result in you then changing the state of the object. The logic you are implementing is probably the called object''s responsibility, not yours. For you to make decisions outside the object violates its encapsulation. Sure, you may say, that''s obvious. I''d never write code like that. Still, it''s very easy to get lulled into examining some referenced object and then calling different methods based on the results. But that may not be the best way to go about doing it. Tell the object what you want. Let it figure out how to do it. Think declaratively instead of procedurally! It is easier to stay out of this trap if you start by designing classes based on their responsibilities, you can then progress naturally to specifying commands that the class may execute, as opposed to queries that inform you as to the state of the object. From pragmaticprogrammer.com/ppllc/papers/1998_05.html I''m too used to designing my models around the data which means I always offer the equivialent of #is_valid and #set_status. Tell don''t ask suggests I should be writing CreditCard#transact and returning an error if necessary. It''s a bit of a sea change for me (and for DHH if I recall his early works correctly!). All part of the heavyweight model thing that seems to be in fashion, perhaps. Best wishes, Jerry