Don't use should #15
Comments
|
+1 for this one. We wrote a small gem that automatically does exactly this: https://github.com/siyelo/should_clean |
|
Probably worth mentioning (and/or linking to) the new expect syntax. |
|
-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
endis clearer than it 'equals 1' do
foo.should == 1
endMoreover, I actually like the semantics that the word "should" brings to the table. I really don't understand why people seem to be pushing the non-use of "should" in spec descriptions. Can someone explain it? |
|
@myronmarston The Rspec |
|
@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 |
|
@myronmarston Interesting; thanks for the reference. I can well believe that a general mixin method would cause more problems than the |
|
@marnen There's "should" in the description and using Using 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: Versus: 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. |
|
@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. |
|
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. |
|
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: ...is much more focused and to the point than output like: |
|
@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". |
|
@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. |
|
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. |
|
@myronmarston +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". |
|
@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?) |
|
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. |
|
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. |
|
Is it just me or does anybody else see that in the "Bad/Good" example section, both examples are the same? Wouldn't the Good section be something like: it 'does not change timings' do
expect(consumption.occur_at).to eql valid.occur_at
end |
|
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:
Andrea Reginato |
|
The following is what I found when checking with a blame 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 |
|
If so, let's fix it. If you have any free time can you send us a pull On Fri, May 24, 2013 at 6:11 PM, idboehman notifications@github.com wrote:
Andrea Reginato |
|
One actually already exists, see #62 -- I made it before I posted the blame. |
|
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! |
|
OMG there is a very very subtle difference between these two, super hard to notice. I thought they were completely equal.
This example is not correct. It's obfuscated. The title should be: Use present tense, third person. |
|
@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: |
|
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 |
|
I think |
|
That's the point though: http://dannorth.net/introducing-bdd/
|
|
@marnen Well that's my opinion since obfuscation is subjective. 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. |
|
@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'? |
|
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:
Although this mainly applies to people like me who scan through the examples quickly before reading thoroughly like they should! 😉 |
|
Why should I not use should syntax (i.e. |
|
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 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)
endEND 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. |
|
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. 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 |
The use of should in the 'it' description is questionable. See more here: betterspecs/betterspecs#15
|
@andypalmer +1 |
|
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. |
|
Must agree with @baweaver. The tests are the blueprint for the code under test and therefore should does not belong in test descriptions.
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:
|
|
class World
def answer
return 42
end
end
# Good, when I search "return 42" I get two results: 1 in the code, 1 in test
it 'should return 42' do
expect(world.answer).to eq(42)
end
# Bad, when I search "return 42" I get one result: in the code
it 'returns 42' do
expect(world.answer).to eq(42)
endI know that I can be smart and always use regex to search my codebase. But this is what I don't do. |
|
The case when the return value is searchable isn't that common I would say. Consider this example: class World
def answer(a)
return a
end
end
it 'should return 42' do
expect(world.answer(42)).to eq(42)
endHow would you search for that and expect a result where both the test and the code shows up? A better approach is to keep your test in the same directory as the code it's testing so that you don't need to search for it. |
|
Agreed with @anulaibar. And even if it is retuning def answer
42
endand it 'returns 42' do
expect(world.answer).to eq(42)
endNow even you must use |
|
yea, maybe it's a bad example. But it's nice when verb is unmodified IMHO. |
|
"Should" is an excellent word to use in tests/examples because it encourages the reader of the test to question whether the test actually does what it say and whether the test should exist at all (refactoring). In my opinion it is a sound attitude to always question tests hence always include "should". |
def answer
42
end
it 'returns 42' do
expect(world.answer).to eq(42)
endMy questioning is, does the test ensure that: If you say that your test ensures that |
|
I would pick avoiding "should", as it is too similar to "should not". I mean which represents negation better?
I think it's the later. |
|
@abelvarga I’d say both represent negation equally well. (I now use the latter form, but for completely unrelated reasons.) |
|
@marnen, I mean the difference is a lot more significant. |
|
Love it that this discussion is still alive after six years ❤️ |
|
@anulaibar Mostly that common practice shifted and I didn’t really feel strongly enough about keeping “should” that I was willing to buck the trend. Also that the tests (if they pass) describe what it does. (I still use “should” in my Cucumber stories, FWIW.) |
|
The change has started: https://www.rubydoc.info/gems/rubocop-rspec/1.2.1/RuboCop/Cop/RSpec/ExampleWording |
|
If you can't convince the world to think like you, why don't you just give them freedom by default? Personally, I use the "should" word a lot, not only in TDD, BDD, but in daily communication with my client, with my colleague. |
|
Being a non-ruby dev, coming from a background where definitions of words are heavily specified (for instance RFC 2119 MUST / SHALL / SHOULD / MAY) and the same words being used in both code and business domains, can someone tell me if this should vs. no-should thing is more about the wording or the (Rspec) syntax? |
|
Hi @Potherca, in my understanding it is not a difference in meaning, it is more of an (IMO) improvement in RSpec's syntax and consequently in wording to make tests easier to read. This is somewhat frequent in the Ruby ecosystem as the language itself tries to make it possible to write code that read similar to how you'd write it in plain English. |
|
Ah, cool. Thanks for the information! |
and others
Write your thoughts about the "don't use should" best practice.
The text was updated successfully, but these errors were encountered: