Some time ago Joe Ferris wrote a piece on the thoughtbot blog called “Let’s Not” (that I recommend you read first),
which concerns itself with the usage of various RSpec helper methods.
subject come under fire and are deemed as problematic.
While I agree with the message of the article, I feel that I could expand on the part about why these helpers are as problematic as Mr. Ferris makes them out to be.
I should start off by saying that while my thoughts pertain specifically to usage of RSpec, a lot of the mentioned structures, methods and patterns are recurrent in other frameworks and languages. I’m sure there’s information here that could be extrapolated to your non-RSpec setup.
When I write a test there are two rules which guide my writing :
- All of the necessary information for understanding what is being tested should be readily available.
- The collaborators in the test should be as minimal as possible.
- Minimize cleverness.
The key thing to remember is that a test is written once but read many more times. If they aren’t on the TODO list for refactoring, then you generally don’t care about your tests while they’re green; it’s when they go red that you start reading them. This is why it’s of importance that when something does break in your system, you want the test to tell you in as straightforward language as possible what broke and why.
Having written tests with and without them for some time now, I find that tools like
subject DRY up your tests, but often at the cost of making the tests less readable.
Examples in the wild
Let’s look at some code to see what I mean.
One of the things I did to learn Rails was to go through Michael Hartl’s Ruby on Rails guide, a book which instructs its readers to write their tests in such a way that it starts to violate some of the aforementioned guidelines.
Before I continue I wish to make it clear that I value the book highly: it helped me tremendously and I definitely recommend it as one of the better introductory books for Rails (at least out of those that I know of). Additionally, despite what I consider being shortcomings in how the test-related portions of the book are written, these portions contain valuable lessons about what to test, so please don’t let this blog post discourage you from reading it.
With that said, there are plenty of examples which illustrate the issues that arise from too extensive usage of the various facilities that RSpec offers you.
User model tests
Let’s start by looking at the tests for the
Due to the size of the file, I’ve removed some tests which I won’t be using to illustrate my point.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68
Let’s assume that the test on line 57 breaks: for some reason the
newer_micropost isn’t included in
The most likely thing you’ll first do is open this spec file and jump down to the line in question. Here’s when the redundantly arduous procedure starts:
Jumping back and forth
First we have to figure out what the
its refers to is.
Let’s backtrack through the code all the way up to line 10 to figure out.
Or well, not really, because you’ll have to read through line 6 to get to know what
@user actually means.
Next, what’s this
We’ll have to jump up to lines 29-31 to figure that out.
Apparently it’s of essence that
newer_micropost is evaluated before each example in which it’s invoked (since
let! is used instead of
This uses the let! (read “let bang”) method in place of let; the reason is that let variables are lazy, meaning that they only spring into existence when referenced. The problem is that we want the microposts to exist immediately, so that the timestamps are in the right order and so that @user.microposts isn’t empty. We accomplish this with let!, which forces the corresponding variable to come into existence immediately.
So, the usage of
let! instead of
let here is completely unrelated to our broken test.
Good to know.
Speaking of which, is it really of relevance to us that the
I would assume not, but considering we’re using this memoized value and not a
micropost created just for the test on line 57, we can only guess.
Oh, I just accidentally noticed that on line 25 apparently the
@user gets saved!
I’m not sure if that changes things but it sure is good to know.
I guess we’ll need to look at some setup to make sense of this:
Lines 51-54 tell us that
follow! something called
Oh, that’s right above at line 49, fine.
Then we create three microposts with a specified content, sure.
Analyzing the journey
To do nothing but understand the full scope of the setup and expectations for the one test on line 57, there’s a lot of jumping around, hunting for the procedure that makes the test happen.
When I’m already in a confused state over why a test tells me that something in my code broke, the last thing I want to do is spend time deciphering the test. I’ve often heard people from the Ruby community talk about using tests as a live documentation for your code, but if I need to put in this much work to read the documentation than I’m starting to think that I might as well read the source code. If the code is well written there will be less to read anyway.
I mentioned previously that I strive to have all the necessary information readily available, where in this case we have a couple of Mystery guests instead.
Confusion was introduced because we had to consider redundant information due to a too large General fixture.
Finally we had the case of needing immediate evaluation of something that is normally lazily evaluated (
let!), increasing “cleverness” of the tests.
Let’s see what this will look like if we try to avoid these helper methods.
Alternate implementation of User model tests
Let’s start off by talking about the
be_admin expectations, since they’re somewhat unrelated to the topic at hand:
First of all, I’m not a fan of testing whether something gets responded to or not.
Test what it does, and if all it does is that it delegates a message somewhere else, ensure that the message is sent.
These tests ensure the existence of an interface, not its workings.
Tests of this kind could be an attempt to ensure that inheritance of certain methods has occurred, so for the sake of brevity we’ll just assume that’s the case (if you check the tests and then the code, you’ll notice that’s not the case for all of them).
Let’s turn that block of tests into a case where I could imagine
subject being used.
Oh, and let’s assume that we have a
User factory available.
I suspect that the reason
@user was instantiated the way it was on on line 6 above, was because it allowed the tests to build an instance of
User but run
@user.save only in the tests where it was necessary (like on line 25).
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25
I think this could be written without
subject as well, but this is a case where it would be an acceptable usage, due to the trivial setup that one needs to comprehend after reading a test that broke.
Let’s now extend this code with the rewritten
describe "micropost associations" block:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65
A number of different things have happened to the tests:
#feed-related tests have been indented out, giving them a dedicated
describeblock in which we clearly test nothing but the
it-block is self-contained and has everything it needs to carry out the test. We’ve avoided the Mystery guest problem.
it-block is written to use a uniform structure of Setup, Procedure and Expectations (in the test on line 25, Procedure was not needed).
- Every collaborating object is as minimal as necessary, assuming that the factories are written as minimal as possible (only to pass validation). We’ve avoided the General fixture problem.
While not entirely related, an issue that I’ve found reoccurring in tests that make heavy use of
before is deeply nested
This spec file is a great example of it (too long to include here).
I remember myself writing these tests when I was going through the tutorial and thinking that it was so clever that you could extend previous scenarios by nesting a
describe inside a previous one.
The error message was extended in a “natural” way, appending just a tiny bit more.
I didn’t need to write particularly large tests either, since they could use all the stuff that was defined a couple of indendentation levels back.
This is enabled by the previously mentioned methods.
It’s also an express train to Confusion Town.
While it might sound cool, it rapidly becomes a frustrating mess to read. It’s essentially a way to exacerbate the already significant potential that the methods have to write confusing tests.
There’s a video on the subject, where the man behind the “Let’s Not” article talks about all of these things. The full video is behind a pay-wall, but if you don’t already have an account on Upcase I do recommend getting one, since they’ve got a lot of good material on the site.
Additionally, let me close by linking to a short but classic post by Jeff Atwood on the subject of how to code.