[rspec-users] lots of nil problems!

Phillip Koebbe phillipkoebbe at gmail.com
Sat Mar 20 14:22:44 EDT 2010

On 2010-03-20 12:17 PM, David Chelimsky wrote:
> On Sat, Mar 20, 2010 at 10:19 AM, Phillip Koebbe
> <phillipkoebbe at gmail.com>  wrote:
>>> I would be glad to help in this way, but it will have to be later today.
>>> And instead of using completely made-up code, I'll use some of my real-world
>>> code and tests as an example. I'll gist them later, unless one of the gurus
>>> comes along and provides enlightenment for you!
>> I was able to get to it sooner than I thought.
>> http://gist.github.com/338707
> Hey Phillip,
> Thanks for taking time to do this. Your examples are clear, and the
> documentation very helpful. They do, however, violate a few guidelines
> that I try to follow and recommend.

Heh, that doesn't surprise me at all. All of my life, I've needed to do 
ABC, so I've figured out a way to do ABC. I frequently learn later, 
however, that I chose a hard way to do ABC and then must learn an easier 
way. Programming has shown itself to not be an exception to this pattern. :)

> Note that I am not trying to change your mind, Phillip, about how you
> should be writing your specs. In the end, the whole purpose of writing
> specs is to help you build a system that you can understand and
> maintain, and provide confidence that the system works according to
> your assumptions by encoding them. For me, that confidence is tied
> directly to my ability to read specs and understand failures quickly,
> which is an admittedly subjective thing.

I appreciate your perspective, David. I appreciate all of the 
perspectives that I see here. Some are presented in a more palatable 
manner than others, but they are all valuable. I don't let much get by 
without considering its place in my own workflow.

> Given that context ...
> 1. Keep setup local
> When an example fails, we want to understand the context as quickly as possible.
> The purpose of nested describe and context blocks is to group things
> together conceptually. While the before blocks happen to support
> cascading (i.e. the ones higher up the nesting chain get run first),
> using them that way makes it much harder to understand the context
> when an example fails. In order to understand why any of these
> examples fail, I'd have to look at the before block in the current
> context, then the before block in the describe block, then at the
> setup methods at the top. That might not seem so bad in a simple
> example like this, but it gets unwieldy quite quickly.

I confess that I have created nestings just to be able to share a before 
block. A typical spec file for one of my controllers is a nested mess, 
and now I know why. Funny that I wrote the silly things but didn't 
realize I was creating such a problem until someone else took a quick 
peek. Oh that I was in a position to pair with someone regularly. That 
would help tremendously.

> 2. Setup context in before blocks, not expectations
> We want the example names to express the intent of each example.
> The before block in the "with valid id' context sets two expectations,
> and then each example sets one of its own. If "should_not_assign_to
> :message" fails because Message didn't receive :find_by_id, the
> failure has nothing to do with the intent expressed by that example.
> Same goes for @message.should_receive(:destroy).
> If you want those expectations to be part of the spec, then add
> examples for them. If you don't care that they are specified, then
> don't. Either way, prefer stub() over should_receive() in before
> blocks.

Good point. By having the expectations in the setup, I also don't get 
the documentation when I run the examples. I didn't think of that before.

> 3. Use expressive names for helper methods
> When I see "before(:each) { setup_admin }", I don't really know what
> that means. In order to figure it out, I have to look up at the
> setup_user, setup_admin, and stub_user methods, and I need to
> understand them all to really understand the purpose of any of them.
> These could (and I'd say should) all be collapsed into this:
> def login_as(type)
>    user.stub(:is_administrator).and_return(type == :admin)
>    User.stub(:find_by_id).and_return(user)
>    session[:user_id = user.id]
> end

I like this much better than what I currently have. As a recovering 
perfectionist, I suffer from the "it's gotta be named just right" 
syndrome. I sometimes spend far too much time trying to figure out the 
best name for this or that, and I've taken to grabbing something that is 
remotely close and moving on so I don't spend my wheels too long.

> Now the before block can say "before(:each) { login_as :admin}" and a)
> I probably don't have to go looking at that method to understand its
> intent and b) I definitely don't have to go looking at three different
> methods.

Of course, *you* might have to wonder what's going on because you're not 
familiar with the project. But since *I* wrote it, I immediately 
understand what setup_admin means. But I still like login_as better ;)

> 4. Clarity trumps DRY
> DRY is a very important principle, but it is often misunderstood to
> mean "don't type the same thing twice." That is not its intent, and
> interpreting it as such often leads to less clarity. The bigger idea
> is that we shouldn't express the same concept in two different parts
> of a system. i.e. if we have two different parts of a codebase that
> validate an email address, for example, and the rules for a valid
> email address change (we decide to ping a service to see if the email
> address actually exists in addition to validating its format), we run
> the risk that we'll forget to change it in both cases. DRY helps us to
> avoid that risk.
> This is a completely different matter from organizing a spec.

This is the rub, I think. Much of the "incorrectness" of my specs can be 
traced back to my effort to stay DRY. I truly appreciate your comment 
about people often misunderstanding DRY to mean "don't retype what you 
don't have to." I'm guilty of that. Admittedly, I have not done 
extensive reading about the topic, but most of what I catch when people 
mention it in forums is just that: keep something DRY by refactoring it. 
I can see that retyping is obviously part of it, but it is really much 
more than that. As with just about everything else in life, one must 
find the balance.

> Consider http://gist.github.com/338767. There are 4 permutations of
> what is essentially the same spec. I'd argue that the two examples
> near the top are far easier to grok than the latter two, even though
> the latter two are DRYer. The first example spells out everything in
> each example with zero indirection and very little API to grok. The
> second one might be a bit DRYer but it requires more API (let).

The first two are most certainly easier on my eyes. That in and of 
itself would be worth changing how I do these things.

[As an aside, I believe you have a copy/paste oops in your "without an 
umbrella" or "gets wet" contexts. Shouldn't it be person.should_not 
be_dry ?]

> 5. Given/When/Then
> This probably shouldn't be last, but ...
> Express every example in the form Given/When/Then: given some context,
> when some event occurs, then the outcome should be ...
> Consider the first example group in http://gist.github.com/338767.
> Both examples in that express the givens (person and day) in the first
> two lines, the event (person walks on the day) on the third line, and
> the outcome on the last line. The symmetry between them makes it very
> easy to see the differences between them, even though that is probably
> the least DRY example in the gist.

I should probably try to use that terminology more in my specs. It's 
proven valuable in the cucumber features, so it should be likewise so in 

> That's probably enough to chew on for now. All comments welcome.

That's a mouthful, for sure!

Thank you, David, for taking the time to generate such a thoughtful 
critique of my work. Not only do I get the benefit of your experience, 
knowledge and insight, but so does everyone following at home.

Patrick, don't listen to me :) Listen to David.

> Cheers,
> David


More information about the rspec-users mailing list