The thing I think differentiates this from other examples (in particular ones I've brought up in this convo), is the relative simplicity and more importantly, the general self-containedness of the examples. For instance:
describe :scene do
subject { parser.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") }
end
That lives at the bottom of the file, and in general, everything I need to know about those specs are within that set of code. Specifically, the main thing being it's mostly self-contained.
I think the main things I personally look for in writing specs is (in approximate value):
1- That it ensures desired functionality works properly
2- That it's deterministic (not flakey)
50- That it's efficient in speed
51- That it's efficient in reader comprehension (the order of these three are dependent on the size of the project and number of developers involved)
55- That it follows standard techniques used in the system (inherently related to 51)
60- That it's easy to update with new changes in functionality (easy to update for maintainenance)
80- That it's self-maintaining
500- That it's DRY
The example given I assume meets 1 and 2 and hopefully 50. I've heard people argue that let is a solution for many of these, but in particular 50 (speed), 51 and 60/80/500 (all of which are some form of a general benefit of DRY code). I'll leave specific responses to that for later, but I'll foreshadow with saying that your tests are poorly laid out if it's a solution for 50, disagree with 51, and I generally prefer standardization paired with search and replace for solving 60/80/500 (maintenance).
Now let's look at what I'd say is a really poor example https://gist.github.com/1378807:
describe "Something doppelganger" do
describe "RailCar" do
let(:gateway) { RailCar.new :login => "a", :password => "b" }
describe "#commit" do
let(:response) { railcar.send :commit, request, {} }
describe "response" do
subject { response }
context "with a REJECTed rail car" do
let(:request) { gateway.send(:build_auth_request, 9011, "token", {}) }
before {
railcar.stub(:parse).and_return(
:requestID => "123",
:vehicleCodeRaw => "N9",
:requestToken => "AFLAKJFLDKJFSLDKFJSLDFKJn129399NANF)(A)FNnnnfnfnnfnf",
:amount => "90.01",
)
}
its(:name) { should_not be_nil }
end
end
end
end
end
The actual test is this:
railcar.send(:commit, request, {}).name.should_not be_nil
or in this example:
its(:name) { should_not be_nil }
I'm Bob, a new developer on the project, I made a change to some code and this test is now failing, so I need to find out why and fix it. Let's say Bob is a solid ruby developer, has done testing in many other languages, and knows enough rspec to know how should and should_not and be_nil work. Let's also give him the benefit of the doubt that he's pretty smart. What does Bob have to do to understand this test failure?
-
Find the line of code that failed:
its(:name) { should_not be_nil }
What has Bob learned from this line of code? Bob knows should_not be_nil, but is confused as to what should_not be_nil, and why this is failing. From reading the full test failure output, he deduces it's :name, and guesses that some magic is going, leading to name being called on something, and that it's not nil.
What has Bob learned so far?
- Some functionality of rspec he didn't know earlier (it/its without a message), and that name should not be nil on something.
- Bob then starts googling around trying to figure out exactly what's going on with this code, and learns there's some rspec magic that makes an it/its without a message work, and some other magic that lets expectations that aren't attached to an object implicitly reference something defined by calling subject with a block.
What has Bob learned so far?
- Some functionality of rspec he didn't know earlier (it/its without a message), and that name should not be nil on something.
- Some more functionality of rspec he didn't know before (subject), and that name should not be nil on something
- So time to track down "subject". Bob knows how contexts/describes work in rspec, so he assumes subject respects that nesting, so starts popping up the call stack for subject.
Bob sees a context "with a REJECTed rail car", with a before and a let, but no subject. Bob doesn't know what let means, but doesn't care right now since it's not relevant. He jumps up the call chain finally finding this line of code:
subject { response }
"Hmm, ok, found subject, but it's referencing a method or something called response."
Bob's smart, so he starts scanning around the file for something called response, and finds:
let(:response) { railcar.send :commit, request, {} }
What has Bob learned so far?
- Some functionality of rspec he didn't know earlier (it/its without a message), and that name should not be nil on something.
- Some more functionality of rspec he didn't know before (subject), and that name should not be nil on something
- That the subject of these tests is somehow a thing called response
-
Now Bob needs to figure out how a method/variable called response is available, so he does a little more googling and/or messing around and finds out let is an rspec thing which is functionally equivalent to having recursive contextual abilities to do:
def response; railcar.send :commit, request, {}; end
What has Bob learned so far?
- Some functionality of rspec he didn't know earlier (it/its without a message), and that name should not be nil on something.
- Some more functionality of rspec he didn't know before (subject), and that name should not be nil on something
- That the subject of these tests is somehow a thing called response
- What let effectively is doing
From here we have 2 options, either 1, we can assume Bob has a highly functioning short-term memory, and can keep all this in his head, or 2, we assume Bob pulls out a text editor and starts tracking things manually. 2 is easier to read about, so let's go with 2.
-
Bob writes out (or puts into his mental buffer) that the failing test is effectively saying:
railcar.send(:commit, request, {}).name.should_not be_nil
What has Bob learned so far?
- Some functionality of rspec he didn't know earlier (it/its without a message), and that name should not be nil on something.
- Some more functionality of rspec he didn't know before (subject), and that name should not be nil on something
- That the subject of these tests is somehow a thing called response
- What let effectively is doing
- What the line of code that is producing a failing result actually is
Note, it took 5 steps to figure out what is actually failing.
One can argue those are 5 steps for a newb, that an advanced user wouldn't need to take, so let's rephrase that as 5 levels of indirection or 5 pieces of knowledge that the test reader needs to process just to figure out the line of code that failed. And Bob/Caitlyn/Dwight still don't know what is causing the failure -- just the line that failed.
Bob doesn't just need to know the line that failed to fix this, he still needs to know what else is going on to fix this.
- Bob starts debugging the failure now, and starts trying to mentally unwrap wtf is going on. Here's what Bob has to mentally map out to debug this test:
The numbering on the arrows represents the order in which you have to traverse the failing line, and are not related to the previous numbering I've listed.
1-4 represent just figuring out what we've talked about so far. 5-6 represent something we haven't even talked about yet, which is how the before relates to the failing line
So let's add 1 more step for Bob to just think through how the before relates to the failing line, and him making sure there are no other befores. And 1 more step for him mentally trying to lay out the full ordering of all the code, or rewriting it in an editor.
So around 8 steps for Bob to understand the situation that's failing. That's a lot to know just to understand one small test.
describe "Something doppelganger" do
it "has an name for a REJECTed rail car" do
railcar = RailCar.new :login => "a", :password => "b"
railcar.stub(:parse).and_return(
:requestID => "123",
:vehicleCodeRaw => "N9",
:requestToken => "AFLAKJFLDKJFSLDKFJSLDFKJn129399NANF)(A)FNnnnfnfnnfnf",
:amount => "90.01",
)
request = railcar.send(:build_request, 9011, "token", {})
response = railcar.send(:commit, request, {})
response.name.should_not be_nil
end
end
This test fails on Bob, what does Bob have to know to fix this test?
-
Bob needs to find the line of code that failed
response.name.should_not be_nil
The response's name is nil.
What has Bob learned so far?
- A response's name is nil
- Bob uses his ruby knowledge (and knowledge of any other language that allows binding of variables and top-to-bottom ordered execution of code) to find what response is, and what it came from.
What has Bob learned so far?
- A response's name is nil
- The entirety of the situation that leads to name being nil
Comparing these 2 versions of the spec:
- the rewrite is 13 less lines of code
- the rewrite has 4 less levels of indention
- the rewrite is linear
- the rewrite is primarily basic ruby + rspec 101 (describe/it + should_not + be_nil)
So what about the straightforward example, if we say the last test fails, what does Ellen (who's a starting from scratch newb like Bob was) have to do to solve it:
describe :scene do
subject { parser.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") } # OH NO, I FAILED!
end
-
Ellen sees an it without a title, so she probably wants to know where the title comes from, but it isn't important to her just fixing the test. Ellen sees a dangling should, and needs to figure out what that is referring to.
-
Ellen does the research Bob did, and finds out about subject
-
Ellen easily finds what the subject is, since it's only 3 lines of code above the actual test (win win win). However, Ellen doesn't know wtf parser is, so she has to go search that down
-
Ellen does a search and finds parser is:
let(:parser) { described_class.new }
She figures that must be related, so she does some research and finds out how let works.
- Ellen either deduces or researches and finds that described_class references TvShowParslet
So now Ellen knows the failing line is:
TvShowParslet.new.should p(" - HDTV.XviD-XOR")
-
Ellen now has to find what p is, so she finds it's
def p(string) parse(string, :trace => true) end
That was pretty easy to find as well, since it's right there at the top of the file. Ellen has no idea how parse works however, and must do a search through the project to find it's definition (or use a fancy editor tool to find the method definition). But that's not a big deal, since she has to do that for all method definitions, and she's pretty awesome at it.
I'll actually stop there, since I can't find where the definition of parse is coming from, and I'm not sure going on would add much value.
Despite me listing a bunch of steps for this example, I think it's much much simpler and pleasant than the previous one. It's one of those gut feelings, just looking at the code and it just feels cleaner, probably since it's easier to read.
What would I change about it?
I'd get rid of the definition of parser
let(:parser) { described_class.new }
I know this is going to be highly controversial, but my reasoning is it's existence breaks the self-containd'ness of the describe blocks, now requiring the reader to have to scan the entire file, including watching out for levels of contexts/describes to make sure it's not being overridden. At a minimum, I'd consider copy-pasting that into the describe blocks.
I'd get rid of saying described_class.new. I'm not really sure what that gains versus TvShowParslet.new, but it definitely adds a bit of rspec specific indirection. I can see described_class.new being useful in a shared example group, but here it just feels to confusing.
I'd rename p to something else, since it doesn't really describe what the method does, and also doesn't seem to really read well to me (definitely just a nit pick though).
To make it fair and not just throw out changes without showing them
0- Original
describe :scene do
subject { parser.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") }
end
1- Movement of the parser line
# Note, that this example also requires a bunch of scrolling
# since you have to go to the top of the file to see what described_class.new
# would be
describe :scene do
let(:parser) { described_class.new }
subject { parser.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") }
end
2- Removal of described_class
describe :scene do
subject { parser.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") }
end
3- Removal of let(:parser)
describe :scene do
subject { TvShowParslet.new.scene }
xit { should p("HDTV.XviD-XOR") }
it { should p(".HDTV.XviD-XOR") }
it { should p(" - HDTV.XviD-XOR") }
end
4- Rename p
describe :scene do
subject { TvShowParslet.new.scene }
xit { should parse_with_trace("HDTV.XviD-XOR") }
it { should parse_with_trace(".HDTV.XviD-XOR") }
it { should parse_with_trace(" - HDTV.XviD-XOR") }
end
Given that this project is using let and the subject thing all over the place, and that these examples are so self-contained, I personally would probably stop at 4.
5- No longer using implicit subject
describe :scene do
xit { TvShowParslet.new.scene.should parse_with_trace("HDTV.XviD-XOR") }
it { TvShowParslet.new.scene.should parse_with_trace(".HDTV.XviD-XOR") }
it { TvShowParslet.new.scene.should parse_with_trace(" - HDTV.XviD-XOR") }
end
OR
5- No longer using implicit subject, but using a before (I don't advocate this one either)
describe :scene do
before { @scene = TvShowParslet.new.scene }
xit { @scene.should parse_with_trace("HDTV.XviD-XOR") }
it { @scene.should parse_with_trace(".HDTV.XviD-XOR") }
it { @scene.should parse_with_trace(" - HDTV.XviD-XOR") }
end
=== Responses to some complaints about the duplication ===
Once you get to step 3 in the above rewrites, you start seeing duplication across describe blocks, in particular, each describe has the subject redefined. Some people freak out about this, saying it's not DRY. I'd personally direct those people to http://blog.jayfields.com/2008/05/testing-duplicate-code-in-your-tests.html and the other posts he did on the matter. I mostly just agree with what he wrote.
I strive for tests to test a single thing, and be self-contained, with a bias towards the future reader, not the future maintainer, or the current writer. Optimally when someone later looks over the test later, they can see exactly what's going on in a few lines of straightforward ruby. And that ruby may even be copy-pasted across tests, but it's setup in such a way that any refactors to the test would just require a simple search and replace job.
If it takes more lines than a couple to fill out a test, or there's a lot of duplication between them, I don't think that's a sign to start using befores or a bunch of esoteric (in the testing world sense) rspec features around it (let/...). I think it's a sign you need to fix the object under test, or you're writing an integration test which falls under different rules.
All in all I totally agree with your write up. My point wasn't to say "see here's a spec that uses it to success so it must always be good" but rather "here's a spec that uses it to success so it must not always be bad". The original bad example that you posted (which I have worked considerably inside) is definitely a terrible spec. I know the history of how it got like that, too, but we can discuss that privately if you care.
That being said, I disagree with removing the
let(:parser)because it is shared for the entire root example group. It's really just an instantiation of the thing we are describing and it's actual definition (i.e. how it was instantiated) does not add any real value to the developer reading the spec. It's name tells you everything you need to know about it for the purpose of working with the spec.On the matter of the
pmethod, it should be namedparsebut the originalparsematcher comes from theparsletlibrary itself and overriding that in specs gives you no nice way to call super. Calling itparse_with_traceis just noise, because the:trace => trueis purely to make the spec failures more verbose, it doesn't affect the behavior of the parser and is therefore not useful to the developer. Ideally, I could just callparseby either setting some default options or overriding it more cleanly, but figuring that out was getting in the way of moving forward so I went withpin the meantime. But yes, it should have a better name that shows the intent.So to the origin of this whole discussion — I disagree with pulling out
letandsubjecteverywhere that it's used, but I am definitely in favor of removing a lot of them and educating people on how to use them correctly.IMO your refactorings of the bad spec above are an improvement because the original usage of
let/subjectwere terrible, not just because it happened to uselet/subject(though the desire to use those keywords — "keyword-driven design" as you put it — are probably the reason the spec looks terrible). The "having to jump around the file" a lot argument is one that is used a lot. There is definitely some truth to it but I think it's not a very strong argument for the same reason that it's not a good one for having god methods in classes. Lots of little, self-describing methods in a class are much better than one big one — in most cases. Iflet/subjectare used and named sensibly, they don't have to take away from clarity overall, and in fact can enhance clarity on a per-spec basis.The key to using these keywords successfully, in my opinion, is to use them to take away the noise from a spec. Specifically in the case of
let, they can be used to remove objects and setup that are needed to make the spec work but are not important in exposing the INTENT of the test case. They can still be used for important things, but in these cases they should be as close to the test as possible (i.e. same example group at least), if not in the example itself. A good rule of thumb forsubjectis to only use it inside an example group whose specs use it and can all be one liners (like my parser specs) — hello, cohesion.Lastly, your refactorings of the parser spec must absolutely stop at step 4. As soon as you do NOT use an implicit subject, an
itneeds to have a name. RSpec generates an example name already for you based on the matcher and implicit subject, but if you don't use it, you'll have nameless examples which don't read very nicely. If you give them names, they more or less repeat the one-line body content and become noise. If you must use an explicit subject, you should probably useexampleinstead ofitto make the examples read more fluidly. At your 5B refactoring, all you are doing is aletbut without that syntax. Having abeforeblock an instance variable provides nothing overletexcept a bit of extra noise.Really lastly, most of your arguments are not really against
let/subjectbut against spreading important details of a spec around. Locality is key for reading specs, and no one should disagree with that. FWIW all your arguments also apply to usingbeforeblocks that have important setup details — which is something a lot of people have been wrongly doing for years.let/subjectis a change that gave opportunity to point fingers. A non-localbeforeblock with instance variables is just as poor as a non-locallet.