On 2010-03-20 12:17 PM, David Chelimsky wrote:
On Sat, Mar 20, 2010 at 10:19 AM, Phillip Koebbe
<phillipkoe...@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
rspec.
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
Peace,
Phillip
_______________________________________________
rspec-users mailing list
rspec-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/rspec-users
