Use subject #7

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

Projects

None yet
@andreareginato
Collaborator

Write your thoughts about the "use subject" best practice.

@cupakromer

RSpec 2.11 has the ability to use a named subject. So your last example can be made better with:

subject(:hero) { Hero.first }
it "should be equipped with a sword" do
  hero.equipment.should include "sword"
end
@myronmarston

The it { should ... } and its(:blah) { should ... } one-liner syntax have there place, but the documentation formatter output for these is often not what you want. It's OK to use them when the output they produce is exactly what you would have typed for the doc string, but otherwise, I think it's a bad idea to use these.

Note also that its will almost certainly be pulled out of rspec-core and moved into an extension gem in RSpec 3, so I think it's bad to recommend it's use at this point.

@nclark
nclark commented Oct 3, 2012

+1

i would consider @cupakromer's code a best practice more than

subject { assigns('message') }
it { should match /it was born in Billville/ }
its(:creator) { should match /Topolino/ }
@cupakromer

You can see David Chelimsky's take on the matter here: http://blog.davidchelimsky.net/2012/05/13/spec-smell-explicit-use-of-subject/

@cupakromer

Thoughts on using subject like this:

describe CommentsController do
  describe '#create' do
    context 'with valid form' do
      let(:expect_commenting) {
        expect{ post :create, comment: { user_id: 1, body: "This is a comment" } }
      }

      subject {
        post :create, comment: { user_id: 1, body: "This is a comment" }
        @controller
      }

      it { expect_commenting.to change{Comment.count}.by 1 }
      it { should redirect_to comments_path }
      it { should set_the_flash[:notice].to 'Comment Created!' }
    end
  end
end
@therealadam

I think this should follow David Chelimsky's advice and advise using let(:object_under_test) rather than subject. Having heavily used subject.should whatever in the past, I think subject is a little too magical to rely on. That said, I don't use it/should as the example above does.

So, bad:

subject { Thing.new }

it "does things" { subject.should do_things }

Good:

let(:thing) { Thing.new }

it "does things" { thing.should do_things }
@cupakromer

@therealadam in your specific example how does the let(:object_under_test) differ in intent from named subject subject(:object_under_test)? Both are lazy initialized. The named subject version allows the flexibility to choose when to use the name and when it makes sense to be implied.

Granted, you probably shouldn't used a named subject for an arbitrary object that doesn't match the describe constant.

@therealadam

Mechanically, they are the same. As Chelimsky points out, using object_under_test.should whatever instead of subject.should whatever is more intention-revealing. The only case he recommends implicit subject is to enable the one-liner syntax.

@andreareginato
Collaborator

Updated the last example and removed the example using its. For the moment I keep suggesting the usage of subject, simply because I've to read the Chelimsky. Anyone can feel free to make a pull request to change the actual definition of the guideline.

@zamith
zamith commented Apr 18, 2013

Subject is basically syntactic sugar for a let, therefore I don't believe there's a big difference in using subject(:thing) or let(:thing), the non named subject is the worst, as it is not semantic at all.

That being said I'm not a fan of using let or subject, and the reason is that when you have nested context or describe blocks and lets in each of them, it easily becomes a quest to find where variables are coming from. Causing a mystery guest

I prefer to use plain ruby methods to explicitly refer to the sut (or any other object), or to group a setup block and then use them as instance variables.

I can live with using let if only in the first level of nesting, and for the one-liner syntax (even though I'm personally not a big fan).

Does anyone agree with this?

@nolastan

Doesn't the example go against Don't use should?

@cupakromer

Most of this is personal / project preferences. So there is no one "proper" way to handle things. If things are working for you, then keep going.

@zamith can you provide an example on how you prefer ruby methods? They suffer from the same sort of "mystery guest" problem when deep nesting is involved. So I'm not sure I see what you are gaining with them.

However, I have seen some crazy test files using let and subjects for everything. Though this usually was because the testing was started by defining everything via let before use (premature extraction). I much prefer writing tests long form (all code / vars in the it block) then doing a refactor step extracting common shared code.

Here's my personal preferences:

  • Use let for memoization of variables which are shared across tests
  • Use subject (named) for the object under test (this provides a semantic clue)
  • Use methods for helper actions and consistent generation of intermediate objects
  • Define other variables that do not meet the above criteria in the test itself as locals
  • Prefer let and subject defined variables to instance variables (barewords vs @vars)
@cupakromer

@nolastan you are correct. Fixed in #57.

@zamith
zamith commented Apr 28, 2013

@cupakromer The ruby methods will be on the outer namespace, so they are global to that file. Also, they must be explicitly called, so there's no mystery guest.

describe "something" do
  before :each do
    @var = 10
  end
  context "inner 1" do
    before :each do
      @var = 5
    end
    it "does something" do
      @var.should eq 5
    end
  end
  context "inner 2" do
    it "does something" do
      @var.should eq 10
    end
  end
end

as opposed to

describe "something" do  
  context "inner 1" do
    it "does something" do      
      var_5.should eq 5
    end
  end
  context "inner 2" do
    it "does something" do
      var.should eq 10
    end
  end
  def var
    10
  end
  def var_5
    5
  end
end

This is a silly example, but it shows that in the first example @var takes different values and is context specific as opposed to the second example, where the var and var_5 are explicitly called and never vary. This being said on regular use cases I would prefer something like:

describe "something" do  
  context "inner 1" do
    it "does something" do
      prepare_variable
      @var.should eq 5
    end
  end
  def prepare_variable
    5
  end
end

which is much more explicit in terms of having a setup phase and thus following the four phase test pattern

@zamith
zamith commented Apr 28, 2013

@cupakromer I agree with most of what you're saying. lets can be really helpful, but if they are not used with care and if they are used across several levels of nesting you're in all kinds of mess.

@cupakromer

@zamith agree with needing to be careful. However, you have the same problem with methods. If you used them across several levels of nesting, you have the same problem. Though this is getting a bit far from the topic of subject here. I'd be happy to discuss further on irc #rspec or in the let issue #8.

@celesteking

that's bullshit. if you write 1 section above that you must use "expect" syntax, then be sure to use that very syntax in this section.

Otherwise people think that "expect" is bad because you make it red boxed. You transition from expect to should, which must not happen.

@andreareginato
Collaborator

If you feel like there are some inconcistencies, please, make a pull
request.
We are more than happy to improve betterspecs.

On Mon, Jun 2, 2014 at 5:54 PM, celesteking notifications@github.com
wrote:

that's bullshit. if you write 1 section above that you must use "expect"
syntax, then be sure to use that very syntax in this section.

Otherwise people think that "expect" is bad because you make it red boxed.
You transition from expect to should, which must not happen.


Reply to this email directly or view it on GitHub
#7 (comment)
.

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

@dideler
dideler commented Jun 13, 2014

The BAD example is

it { expect(assigns('message')).to match /it was born in Belville/ }
it { expect(assigns('message').creator).to match /Topolino/ }

while the GOOD example is

subject { assigns('message') }
it { should match /it was born in Billville/ }

The GOOD example is missing the second test of the creator.

@diegoguemes

I just visited this thread to check if more people considered the use of the word subject an aberration inside the body of an spec. I'm so glad more people think in the same way! 😄

Just for clarifying, I'm totally ok with the subject syntax, I disagree with the use of subject without a named variable, because it doesn't express any intent.

The post http://blog.davidchelimsky.net/blog/2012/05/13/spec-smell-explicit-use-of-subject/, which some people suggested, is a great read

@andreareginato
Collaborator
@onebree
Contributor
onebree commented Oct 23, 2015

@diegoguemes yes, please submit a pull request, including that blog link in the comments!

@maximveksler

It seems that RSpec official docs recommend subject not be used in user facing examples.

While the examples below demonstrate how subject can be used as a
user-facing concept, we recommend that you reserve it for support of custom
matchers and/or extension libraries that hide its use from examples.

https://www.relishapp.com/rspec/rspec-core/v/3-5/docs/subject/implicitly-defined-subject

Am I getting this wrong ?

@dideler
dideler commented Oct 20, 2016

@maximveksler note that the page is about the implicit subject. I wonder what their standpoint is on the explicit (i.e. named) subject.

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