Don't use should #15

Open
andreareginato opened this Issue Oct 3, 2012 · 47 comments

Projects

None yet
@andreareginato
Collaborator

Write your thoughts about the "don't use should" best practice.

@glennr
glennr commented Oct 3, 2012

+1 for this one. We wrote a small gem that automatically does exactly this: https://github.com/siyelo/should_clean

@myronmarston

Probably worth mentioning (and/or linking to) the new expect syntax. should will almost certainly never be removed from rspec, but it's good for folks to understand that it's prone to producing weird failures on proxy objects, and the expect syntax avoids that problem entirely.

@marnen
marnen commented Oct 3, 2012

-1. I know this is popular, but I think it's at best neutral, and at worst an actively bad idea.

I think the parallelism between description and code in

it 'should equal 1' do
  foo.should == 1
end

is clearer than

it 'equals 1' do
  foo.should == 1
end

Moreover, I actually like the semantics that the word "should" brings to the table. it 'should equal 1' means, to me, that the thing being tested should equal 1, and if it does not, then we have a problem. it 'equals 1' seems to me like it means that the thing does equal 1, even if the test fails. To me, that's less clear.

I really don't understand why people seem to be pushing the non-use of "should" in spec descriptions. Can someone explain it?

@marnen
marnen commented Oct 3, 2012

@myronmarston The Rspec should method creates problems? I can't say I've ever run into that. I hope it stays viable; for most things, I like the should syntax better than the expect syntax.

@myronmarston

@marnen -- Read my blog post on the topic. It doesn't usually create problems, but on certain kinds of objects (delegate/proxy objects) you can get weird, surprising failures. A number of users have run into these issues. As I explain in that blog post, we don't plan to ever remove the should syntax but we do plan to disable it by default at a future point (it'll be a simple config change to re-enable it if you want to keep using it).

@marnen
marnen commented Oct 3, 2012

@myronmarston Interesting; thanks for the reference. I can well believe that a general mixin method would cause more problems than the expect block syntax.

@hosh
hosh commented Oct 3, 2012

@marnen There's "should" in the description and using should() in the spec. This best practice refers to using "should" in the description, and not necessarily should()

Using should() does create problems, and expect() is a better solution. But we are talking about the description passed to it().

The community is divided on this one. The main arguments I see against using "should" in the description of expectation usually come down to how "should" does not match the RFC 2119 keywords (http://www.ietf.org/rfc/rfc2119.txt). That documents the convention used by internet standards document. "SHOULD" indicates a recommended specification. The argument goes on to say, the way we use rspec, we are actually using "MUST". When an expectation fails, rspec tags it as a failure. The intention is to go fix it.

I learned Rspec in the early days when using "should" was considered best practice. It was not intended to conform to RFC 2119. You're not writing assertions or specifications so much as expectations. You think something is going to happen, and then you test against it. Pass or fail? Using "should" makes much more sense when you write what you expect to happen before writing the code first, rather than writing the code after the fact.

The biggest advantage of using "should" is that when you view the fully-formated output, you can key off of the "should" as a delimiter:

GET /users
  when authorized
    should respond with 200 OK
    should respond with JSON
    should respond with an index of users
  when unauthorized
    should respond with 403 Forbidden
    should not respond with an index of users
    should log the request

Versus:

GET /users
  when logged in
    responds with 200 OK
    responds with JSON
    responds with an index of users
  when logged out
    responds with 403 Forbidden
    does not respond with an index of users
    logs the request

To form a negative off of third-person singular, you have to use something else. It breaks up your ability to scan down the specs. When you have a number of these jammed up together, you now spend more time parsing it.

But not everyone cares about actually reading the output of the specs. The customers who might read it just care whether it is all green or not. Developers tend to just look at the failing specs and go straight to the code.

I think it comes down to which itch you are scratching.

@marnen
marnen commented Oct 3, 2012

@hosh Yeah, for me it's an old habit from the early days too. I never thought about the RFC 2119 connection, but I can't say that makes much difference for me in this case. :)

And you're right that using "should" seems to make more sense when you write the specs first -- which we should all be doing! :)

As far as not reading the output...IMHO that's just silly. If you're not reading the output of your specs, why are you bothering with them in the first place?

Very good points.

@andreareginato
Collaborator

The motivations related to this guideline is quite simple. I want my test to do that thing. If it does not it fails.

I also like more the second version of the rspec output written from @hosh because it directly tell you what your app does, and not what it should be doing. It's a slightly but important difference in my pont of view.

I also have to admit that yours are all good points.

@myronmarston

FWIW, my motivation for not putting "should" at the start of all my doc strings is that it amounts to noise when reading the output if each and every example starts with that word.

Reading output like:

Widget
  sends a message
  cleans up after itself
  flibets the fizzbuzz

...is much more focused and to the point than output like:

Widget
  should send a message
  should clean up after itself
  should filbet the fizzbuzz
@hosh
hosh commented Oct 11, 2012

@andreareginato and @myronmarston this is exactly what I mean by "what itch do you want to scratch". For @marnen and I, the presence of "should" acts like a visual delimiter. I have met and worked with other developers like you, where that is perceived and treated as noise -- it viscerally bothers them to see it there, though it is usually framed in terms of correctness. (But that is what "scratching the itch" means ... being able to quickly identify one-off errors). Not having a visual delimiter is more distracting for me and others like me.

@andreareginato to your point, "I want my test to do that thing. If it does not it fails." is an interesting statement. To me, that kind of black-and-white guarantee is an illusion. At the end of the day, for all the tests you write, you don't actually know if that is what the application is doing. You can approach certainty asymptotically, and at some threshold, you make a leap of faith.

I think of writing the tests with "should" as a declaration of intent, with my best effort at making it happen. If you want to see what the code is "actually" doing, you read the code -- and you make sure the spec code reads well.

What it comes down to: not prefixing the description of "should" should not be considered "best practice". There are clearly developers who disagree on this. Much of it comes down to how you interact with the world and your personal philosophies. Consider that how using "should" irritates you -- there are just as many developers for whom, not using "should" irritates them. We can talk about whether one is more correct or not, but it comes down to this irritation.

Instead, I think it is better to reframe this whole "best practice" to --> "Whether you use 'should' or not, use a consistent format and convention in your specification descriptions and stick with it".

@pepe
pepe commented Oct 11, 2012

@hosh I can actually see your point and understand it, but in my personal view, less code is better. And yes even descriptions are code for me.

Only my two cents tho.

so +1 on this.

@myronmarston

I think the take away from this conversation is that experienced RSpec users disagree on this point. I think there's reasonable disagreement about many of the guidelines on the site. I would love if it the site evolved to discuss the pros/cons of the different approaches, rather than stating a binary "good"/"bad" split. Even if everyone agrees on a particular guideline, it's important that people understand why the guideline is a good idea, and not follow it blindly.

@hosh
hosh commented Oct 16, 2012

@myronmarston +1
@pepe +1

I suppose that means that "best practice" is more about inspiring craftsmanship rather than setting standards. Telling people, "hey, really look at what you are doing".

@andypalmer

@myronmarston I tend to write my specs like this, to avoid the noisy repetition, but keep the less imperative version (oh, this should equal 1, should it?)

WidgetShould
  send a message
  clean up after itself
  filbet the fizzbuzz
@andreareginato
Collaborator

I've added the the link to the new expectation syntax as suggested from @myronmarston.

About the @myronmarston complain about the bad attitude to say what is "good"/"bad", I agree. A solution could be adding a "Why" section describing why we say what we say for every guideline. A pull request will be enough to start.

Any correction and comment to the updated guideline is appreciated.

@mark-rushakoff

Lots of good discussion in this thread about starting specs with should.

If you're on the side that no specs should ever begin with should, you might find the should_not gem I've written helpful for enforcing that opinion on your project.

@idboehman

Is it just me or does anybody else see that in the "Bad/Good" example section, both examples are the same?

BetterSpecs Typo

Wouldn't the Good section be something like:

it 'does not change timings' do
  expect(consumption.occur_at).to eql valid.occur_at
end
@andreareginato
Collaborator

We need to check with a blame where something went wrong. Nice found!

On Fri, May 24, 2013 at 5:16 PM, idboehman notifications@github.com wrote:

Is it just me or does anybody else see that in the "Bad/Good" example
section, both examples are the same?

[image: BetterSpecs Typo]https://a248.e.akamai.net/camo.github.com/7ab359c0146686e7e17de9f3148b93439bb34576/687474703a2f2f692e696d6775722e636f6d2f70766c703630472e706e67

Wouldn't the Good section be something like:

it 'does not change timings' do
expect(consumption.occur_at).to eql valid.occur_atend


Reply to this email directly or view it on GitHubhttps://github.com/andreareginato/betterspecs/issues/15#issuecomment-18411107
.

Andrea Reginato
Lelylan | reThink your house
http://lelylan.com

@idboehman

The following is what I found when checking with a blame

iboehman@persaeus:~/Git Repositories/betterspecs/content$ git blame -L 755,771 index.html
818f1df9 (Jasdeep Singh   2012-10-03 11:42:47 -0300 755) <p class="wrong">bad</p>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 756) 
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 757) <div>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 758) <pre><code class="ruby">it 'should not change timings' do
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 759)   consumption.occur_at.should == valid.occur_at
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 760) end
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 761) </code></pre>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 762) </div>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 763) 
d30d340a (Andrea Reginato 2012-10-03 13:19:02 +0200 764) <p class="correct">good</p>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 765) 
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 766) <div>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 767) <pre><code class="ruby">it 'does not change timings' do
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 768)   consumption.occur_at.should == valid.occur_at
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 769) end
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 770) </code></pre>
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 771) </div>

Sot it seems to have been incorrect since the section was added, or at least, that's what I'm gathering from looking at 51db634

@andreareginato
Collaborator

If so, let's fix it. If you have any free time can you send us a pull
request @idboehman?

On Fri, May 24, 2013 at 6:11 PM, idboehman notifications@github.com wrote:

The following is what I found when checking with a blame

iboehman@persaeus:~/Git Repositories/betterspecs/content$ git blame -L 755,771 index.html
818f1df (Jasdeep Singh 2012-10-03 11:42:47 -0300 755)

bad


51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 756)
51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 757)

51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 758)
it 'should not change timings' do
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 759) consumption.occur_at.should == valid.occur_at
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 760) end
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 761)

51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 762)

51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 763)
d30d340 (Andrea Reginato 2012-10-03 13:19:02 +0200 764)

good


51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 765)
51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 766)

51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 767)
it 'does not change timings' do
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 768) consumption.occur_at.should == valid.occur_at
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 769) end
51db6346 (Andrea Reginato 2012-10-01 18:19:03 +0200 770)

51db634 (Andrea Reginato 2012-10-01 18:19:03 +0200 771)

Sot it seems to have been incorrect since the section was added, or at
least, that's what I'm gathering from looking at 51db63451db634#L0R568


Reply to this email directly or view it on GitHubhttps://github.com/andreareginato/betterspecs/issues/15#issuecomment-18414536
.

Andrea Reginato
Lelylan | reThink your house
http://lelylan.com

@idboehman

One actually already exists, see #62 -- I made it before I posted the blame.

@andreareginato
Collaborator

Giving a better looking the code is correct.

# bad (with should) 
it 'should not change timings' do
# good (without should) `
it 'does not change timings' do 

Sorry for not checking it as soon as you wrote the comment, and thanks for the patch!

@rebelwarrior

OMG there is a very very subtle difference between these two, super hard to notice. I thought they were completely equal.

it 'does not change timings' do
it 'should not change timings' do

This example is not correct. It's obfuscated.
Not only that, neither of the strings explain what is being tested very well.
This example should be removed or altered for clarity.

The title should be: Use present tense, third person.
The explanation: Avoid using imperative words like 'should' in your test description since they can be taken out of context more easily.

@marnen
marnen commented Aug 13, 2013

@rebelwarrior: On what basis do you call the example obfuscated? The only thing at issue here is the use of the word "should" in spec description text, so the example provides a minimal pair demonstrating just that.

As far as your proposed change, it won't work. "Should" is grammatically also third-person present.

@idboehman:
Let's not drag the expect syntax into this one. That's a separate issue.

@marnen
marnen commented Aug 13, 2013

I should mention that also, consistent with my earlier comments on this subject, I wouldn't mind if this "guideline" were removed from Better Specs altogether. I think removing "should" hurts clarity with RSpec. (OTOH, I just tried MiniTest::Spec for the first time and found that its must syntax seemed to be clearer when the descriptions didn't contain "should". Hmm.)

@warmwaffles

I think must is super clear. It's very strict and straight forward. should sounds like it has a chance of failing.

@andypalmer

That's the point though: http://dannorth.net/introducing-bdd/

A more subtle aspect of the word should becomes apparent when compared with the more formal alternatives of will or shall. Should implicitly allows you to challenge the premise of the test: “Should it? Really?” This makes it easier to decide whether a test is failing due to a bug you have introduced or simply because your previous assumptions about the system’s behaviour are now incorrect.

@rebelwarrior

@marnen Well that's my opinion since obfuscation is subjective.
But let's check the definition of obfuscated:
render obscure, unclear, or unintelligible (from the Mac Dictionary, btw)
I do believe that is unclear. Because should is a method in rspec, and one that has an alternate syntax (expect) while this is really referring to the description text. So while it's my opinion, it's not unfounded.

Additionally, I disagree with this guideline, but that's besides the point. It took me a second look just to notice the difference between the examples and that defeats the purpose of an example. it { should be_clear }

@marnen
marnen commented Aug 13, 2013

@rebelwarrior The example is a minimal pair. The only difference between the two code snippets is the exact difference being discussed. How could it be any clearer?

Perhaps we should change the title to 'Don't use "should" in description text'?

@marnen
marnen commented Aug 13, 2013

@warmwaffles Of course "should" has a chance of failing, just as any spec does. If we knew that a spec would always pass, then it would give us no useful information and we would not write it.

@stefanhendriks

I would try to keep away from does not and focus what you want to know. So, I would rephrase it something like this:

it 'keeps change timings' do
or (I don't know what the context is of this test exactly)
it 'keeps timings' do

@karlentwistle

should this still be using the == syntax? I tried to run a similar spec and got

The expect syntax does not support operator matchers, so you must pass a matcher to `#to`.
@almassapargali almassapargali pushed a commit to almassapargali/betterspecs that referenced this issue Dec 30, 2013
Almas Sapargali Update ru.html
Seems like russian version not update, look at lelylan#15 (comment)
2b7890e
@baweaver
baweaver commented Jan 6, 2014

With the switch to expect syntax, it makes it very bad grammar to do one-liner it statements with just expect:

# BAD
it { expect(1).to eq(1)}

#GOOD
it 'should be one' { expect(1).to eq(1) }

it 'should be one' do
  expect(1).to eq(1)
end

As the entire point of RSPEC is to be human readable, that really does contradict the spirit of the framework to do things in such a manner. Thoughts?

@pshields
pshields commented Jan 6, 2014

I disagree with @baweaver; I think the small amount of cognitive dissonance required to accept the wrong plurality of the word "expect" is worth the elimination of redundancy in the it-block description.

# good
it { expect(my_method).to be_true }

# bad
it 'returns true' do
  expect(my_method).to be_true
end
@meesterdude

I disagree with @pshields! :-p

There is just no way someone is going to look at this

it { expect(my_method).to be_true }

and nod their head in agreement that this is good code.. The great strength of ruby is it's english like syntax and good code reads like a good sentence. That's a horrible sentence, no way around it. Maybe in another language it wouldn't matter so much, but this is ruby.

You're of course to free to write specs that way if you really want; we all have to make sacrifices to get the LOC down. but I would never sacrifice readability.

I'm also not sure how the rspec output would look with that in documentation. I'd assume just as funky and out of place.

@marnen
marnen commented Jan 6, 2014

@pshields I think @baweaver is right on this one. If you really want to do that, it's better to alias expect to expects.

Besides, the should syntax isn't going away, so you can still use it. I think something like the following may be optimal:

it 'has an explicit description' do
  expect(x).to == 1
end

it { should == 5 } # no explicit description

If you really need expect with the short syntax, also remember that specify is an alias for it, and may be less jarring in this context.

Also, I find that the it block syntax, while convenient, is easy to abuse. It only works when you're expecting something to be constant, but I see it used a lot for computed values. Those need an explicit description anyway, so that the spec description serves as proper documentation. So:

subject { sum(x, y) }

context 'x and y are nil' do
  let(:x) { nil }
  let(:y) { nil }
  it { should be_nil } # fine, saves typing an explicit description 
end

context 'x and y are not nil' do
  let(:x) { rand 100 }
  let(:y) { rand 100 }

  # WRONG:
  it { should == x + y } # because spec description in report will just say "it should == 78", without actually documenting why

  # RIGHT:
  it 'should return x + y' do
    expect(subject).to == x + y
  end
end
@marnen
marnen commented Jan 6, 2014

And apparently you'd have to use eq rather than == in my previous example, but you get the idea.

@myronmarston

If you really want to do that, it's better to alias expect to expects.

expects reads well grammatically, but makes no sense. Consider:

describe 3 do
  it { expects to be > 2 }
end

When you use expects, you are saying the subject (which is what it is meant to refer to) expects something, but that's backwards: you expect something about the subject, not vice-versa.

Anyhow, I personally cringe when I read it { expect(object).to matcher }. YMMV, of course. But rspec provides the specify alias that I think reads much better:

specify { expect(object).to matcher }

RSpec 3.0.0.beta2 (not yet released; we hope to get it out soon) has a new method to help with expect-based one liners:

it { is_expected.to matcher }

is_expected is defined as expect(subject).

All that said: even if you disable the :should syntax, it is still allowed in one-liners:

it { should matcher }

That's because disabling :should is meant to remove the monkey patching of should from Object, but the should for one-liners is defined separately and doesn't involve any monkey patching. When we introduced the expect syntax, we discussed the one-liner syntax and at the time couldn't come up with an expect-based syntax we were happy with for it (we thought up is_expected later) -- so we decided to leave the definition of should for one-liners in place, regardless of the config. You can read the original conversation here if you're interested:

rspec/rspec-expectations#119 (comment)

Finally, I'd like to emphasize that while one-liners are a nice feature when your doc string and your matcher expression are identical, being as terse as possible using one-liners is not the point of RSpec. I see a lot of questions (on stackoverflow and elsewhere) that assume that "idiomatic" RSpec uses as many one-liners as possible -- but I (as maintainer of RSpec) use them very, very rarely.

@trinitronx

Seems there are 2 arguments going on here about the usage of should:

  1. Using "should" in the description
  2. Using the should DSL vs. expect

On argument 1 @hosh makes a good point:

What it comes down to: > not prefixing the description of "should" should not be considered "best practice". There are clearly developers who disagree on this. Much of it comes down to how you interact with the world and your personal philosophies. Consider that how using "should" irritates you -- there are just as many developers for whom, not using "should" irritates them. We can talk about whether one is more correct or not, but it comes down to this irritation.

Instead, I think it is better to reframe this whole "best practice" to -> -> "Whether you use 'should' or not, use a consistent format and convention in your specification descriptions and stick with it".

My feelings: Use whatever makes sense to you in your description text. Arguing about this reminds me a bit of the Lilliput and Blefuscu argument in Gulliver's travels about which end of the egg to crack.

On argument 2, the only practical reason to not use the should DSL seems to avoid certain errors with proxy objects and potentially if it ever becomes deprecated.

@adbatista adbatista added a commit to adbatista/betterspecs that referenced this issue May 20, 2014
@adbatista adbatista Update br.html
fix syntax for BR version, related #15
d21a864
@audionerd

I was initially confused, thinking this was the same as the heading Expect vs Should syntax. The confusion I think was due to the example showing both a change in description, and a change in the spec syntax, so at first I didn't realize this was about the description of the spec (until I read closer).

A better heading at a glance might be:

Don’t use the word “should” in descriptions

Although this mainly applies to people like me who scan through the examples quickly before reading thoroughly like they should! 😉

@hellboy81

Why should I not use should syntax (i.e. should.js in JavaScript) ?

@baweaver
baweaver commented Feb 3, 2015

That one more of got shoehorned into this. The original point was that using one liners only saves you space, and shouldn't be used frequently.

START OPINION RANT

On to the second point, and this is highly personal in preference as to using should in description strings. I don't like it quite simply because you're not certain. It should do something? It either does or it doesn't, and using flimsy wording doesn't make a concrete description. Granted that's soapboxing, but I still dislike seeing tests that lack a read of certainty to them. You need to be confident in your specs, not timid.

As an example:

# Bad - Timid, and a fallback to old syntax
it 'should return 3 when given 1' do
  expect(method(1)).to eq(3)
end

# Good - Actionable, strong verb describing the test.
it 'returns 3 when given 1' do
  expect(method(1)).to eq(3)
end

END OPINION RANT

@hellboy81 - Mate, hate to break it to you but this is RSPEC, not Jasmine / Should.js. General concepts apply, but definitely the wrong area to debate this one.

@andypalmer

Dan deliberately chose "should" when creating BDD to introduce the element of uncertainty; specifically with regards to our understanding of the system we are specifying.
"It should return 3 when given 1" is effectively short-hand for "based on our current understanding and assumptions, we expect method(1) to return 3. If, at some point in the future, this is not the case, we should be prepared to accept that our understanding was perhaps incomplete and that this expectation is no longer true"

The problem with the assertive case is that it invites no (explicit) doubt. If it fails, then the implementation must be broken.

It's a small distinction; and competent developers will implicitly question their assumptions, but Dan created BDD as a language-shift to help bring those whose understanding was not in that place up to speed in an simpler way. This is why the best TDDers see no difference between TDD and BDD and why beginners write better specifications when given BDD thinking tools

@chrisarcand chrisarcand referenced this issue in rspec/rspec-expectations Apr 14, 2015
Closed

Emphasize expect syntax in generated descriptions #769

@lmatiolis lmatiolis added a commit to lmatiolis/catarse that referenced this issue May 13, 2015
@lmatiolis lmatiolis Change expectation message.
The use of should in the 'it' description is questionable.
See more here: lelylan/betterspecs#15
0c2c82c
@tgaff
tgaff commented Jan 22, 2016

I agree with audionerd.

This specification is about the test description the code inside should be the same. There's already a separate entry for should vs. expect syntax.

@anulaibar

Must agree with @baweaver. The tests are the blueprint for the code under test and therefore should does not belong in test descriptions.
To quote Wikipedia:

A unit test provides a strict, written contract that the piece of code must satisfy

The word must is key here. Test descriptions serve as documentation of the code and should be clear and precise without room for interpretation. There is no maybe. Either the code does it or it doesn't do it.

To quote Yoda:

Do. Or do not. There is no should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment