How to describe your methods #2

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

Projects

None yet
@andreareginato
Collaborator

Write your thoughts about the "how to describe your methods" best practice.

@farski
farski commented Oct 3, 2012

Why use (dot) classMethod rather than ::classMethod? It seems like in other places where the distinction is made with symbols (eg, ruby doc), it's :: and #

@tilsammans

For me this notation is too technical. RSpec was created to express specs/tests using human language.

@MattRogish

+1 to ::

@jherdman
jherdman commented Oct 3, 2012

@farski I think that's a matter of taste given that both :: and . are acceptable notation for class methods. It's probably more important that your team agree on one or the other :)

@tilsammans depending on the context of the examples, I agree. When dealing with small units (e.g. a model), I tend to focus on describing the behaviour of individual methods. When dealing with something like a Rails controller, I'll describe the behaviour of the request, in which case I'd do something like this:

describe "GET #new" do
  # ...
end

describe "POST #create" do
  context "an admin user" do
  end

  context "an authenticated user" do
  end
end
@Spaceghost

How I write specs

This is how I've been writing my specs lately. I'm intentionally deviating from the norm to explore and find out if I can find some better practices. One of my goals is to explore writing a formatter that outputs rdoc from passing tests.

require 'spec_helper'
require 'blocking_spec_helper'
require 'persistence/post'

describe 'app/persistence/post.rb' do

  before do
    @post = ::Persistence::Post.new title: Faker::Name.name, body: Faker::Lorem.paragraph, author: "Johnneylee"
  end

  describe ::Persistence::Post, 'validates' do
    %w[title body author].each do |attribute|
      specify "presence of #{attribute}" do
        setter = attribute + "=" 
        @post.send setter.to_sym, nil
        @post.should be_invalid      
      end
    end

    specify 'uniqueness of title' do
      title = Faker::Name.name
      ::Persistence::Post.create! title: title, body: Faker::Lorem.paragraph, author: Faker::Name.name
      @post.title = title
      @post.should be_invalid
    end
  end

  describe ::Persistence::Post, 'methods' do
    describe "#author" do
      it 'should be a string' do
        @post.author.should be_an_instance_of String
      end
    end
  end
end

Which outputs this.

app/persistence/post.rb
  Persistence::Post validates
    presence of title
    presence of body
    presence of author
    uniqueness of title
  Persistence::Post methods
    #author
      should be a string

Finished in 0.70489 seconds
5 examples, 0 failures

Closing

I want to achieve a level of bliss where I can write expressive specs that output real documentation that can be used in a release.

@binarycode

@Spaceghost the it 'uniqueness of title' do parts look quite ugly, i think it's much clearer to write specify 'uniqueness of title' do instead

@Spaceghost

@binarycode I definitely agree. I wrote a small method that just wraps #it and I use that. I called it #specify as well. But it seems it was already in rspec!

@dchelimsky

@Spaceghost would you mind editing your comment above to use "specify" instead of "it" in that case? It'll help me sleep at night. Same with it "presence of attribute". Thx.

@evanphx
evanphx commented Oct 6, 2012

If your describe is ONLY a method name, like describe ".admin?" then you're not longer describing behaviors, you're now writing unit tests.

The point, generally, is to describe functionalities in terms of observable behaviors and then the code shows how those behaviors are manifested. Here is an example:

describe "Admin user" do
  before :each do
    @user = AdminUser.new
  end

  it "should be detectable" do
    @user.admin?.should be_true
  end
@bobbytables

Just a general rule of thumb I have is to never use "should" in my example. Your spec is saying what it does, not what it should be doing.

it "validates presence of name"

@dchelimsky

@evanphx consider:

describe Stack do
  context "when empty" do
    it "returns nil for #peek"
  end
end

describe Stack do
  context "when empty" do
    describe "#peek" do
      it "returns nil"
    end
  end
end

describe Stack do
  describe "#peek" do
    it "returns nil when empty"
  end
end

Output:

Stack
  when empty
    returns nil for #peek

Stack
  when empty
    #peek
      returns nil

Stack
  #peek
    returns nil when empty

Each of these approaches has pros and cons and I can't really argue that one of them is always correct or incorrect, but I think the last one, which uses describe "#peek" is perfectly reasonable due to the context of describe Stack.

@Spaceghost

@dchelimsky The funny part was that I have this in my common_spec_helper.rb in a private repo for work.

def specify description, &blk
  self.send :it, description, blk
end

I never saw the #specify or #example methods before. Thanks for being a champ.

@Spaceghost

@evanphx While I agree that you should be describing the behaviours of your objects rather than the implementations in your describe blocks, I'm intentionally not doing that so I can explore outside of the recommended use cases.

@dchelimsky how do you feel about how I was writing my specs above? Too weird?

Maybe there's a good point in here. If I wrote proper behavioral specs and then unit tests separately, that might do me a bit of good in terms of separating out capturing the domain.

@dchelimsky

@Spaceghost I'd let "validates" show up at the front of the validation examples - I didn't see "validates" at first and didn't understand what specify "presence of title" meant. Also, the only case in that example that maybe shows what a valid post looks like is "uniqueness of title", because it happens to use create!, and I think that means it'll raise an error if it doesn't get valid data.

Here's what I'd write for the same set of examples:

require 'spec_helper'
require 'blocking_spec_helper'
require 'persistence/post'

describe ::Persistence::Post do
  def build_post(overrides={})
    ::Persistence::Post.new { title: Faker::Name.name, body: Faker::Lorem.paragraph, author: Faker::Name.name }.merge(overrides)
  end

  def create_post(overrides={})
    build_post(overrides).save!
  end

  it "is valid with valid attributes" do
    build_post.should be_valid
  end

  %w[title body author].each do |attribute|
    it "validates presence of #{attribute}" do
      build_post( attribute.to_sym => nil ).should_not be_valid
    end
  end

  it 'validates uniqueness of title' do
    pre_existing_post = create_post
    build_post(title: pre_existing_post.title).should_not be_valid
  end
end
@dchelimsky

@Spaceghost I accidentally hit submit and had to go back and edit that (a few times). Looks like what I'd do now.

@pepe
pepe commented Oct 11, 2012

@dchelimsky thanks for array of attributes example, but should not there be one more end after dynamic example.

And for describing methods, I am on the @evanphx side. I am always trying to describe behavior. Except for things like it { should be_empty }.

@dchelimsky

@pepe - yep, fixed, thanks.

@dchelimsky

@pepe, @evanphx I want the descriptions to give me an overview of what the behavior is and the code examples to give me the detail, but "overview" means different things in different contexts. For example, I would say "Money supports addition using +" rather than just "Money supports addition" because I can read that in the output and learn quite a bit of useful information from that. I would be very unlikely, however, to cite a specific examplee.g. "Money returns $5 for $4 + $1".

That said, I think describing a specific return value is reasonable when talking about a behavior like "Array[index bigger than size] returns nil" it describes the Array's behavior in response to a class of inputs.

@andypalmer

I originally disagreed with this recommendation, then I read @dchelimsky's post and thought "Hmm, this works well for documenting APIs".
I'm not a big fan of RDOC, JavaDoc etc, and I'm still on the fence about this one. Perhaps it would be less controversial if the recommendation were titled "How to describe your public API"?

@Spaceghost

@dchelimsky I know you've handed the reins over to @alindeman and @myronmarston for rspec, but I really like your methods #build_post and #create_post.

I'm going to do some braining on this, partly with my fingers, and see what rises out of it.

Thank you again for all the love/hate/beer you've had in the name of RSpec.

@dchelimsky

@Spaceghost I actually picked that up from somebody else. When I remember who I'll try to also remember to post back here to give credit where credit is due.

@zolzaya
zolzaya commented Jan 29, 2013

👍

@sferik sferik referenced this issue in intridea/multi_json Feb 12, 2013
Merged

Some small changes before 1.6.0 release #85

@tony612 tony612 added a commit to tony612/betterspecs that referenced this issue Mar 19, 2013
@tony612 tony612 finish #2 a558b74
@brett-richardson

I just wrote a small gem that allows a much more terse syntax in this very situation.

https://github.com/brett-richardson/rspec-describe-method

The syntax is like this:

describe 'test' do
  describe_method '#upcase' do
    it{ should eq 'TEST' }
  end

  when_calling '#concat', 'ing' do # Alternative syntax (and arguments)
    it{ should eq 'testing' }
  end
end

Essentially you can infer the value of subject by using the following call: describe_method "#upcase" do
The gem detects instance and class methods and delegates them to the required object.

Essentially, under the hood... this is happening:
subject{ super().send method_name[1..-1], *args } (when it's an instance method).

Allows some really nice specs. Also allows nesting.

Does anyone see any value in this? Let me know.

@mkaschenko mkaschenko added a commit to tdd-tales/Charles_Perrault-Puss_in_Boots that referenced this issue Jul 9, 2014
@mkaschenko mkaschenko Make Tale::Son use Tale::Inheritance f65691f
@mkaschenko mkaschenko added a commit to tdd-tales/Charles_Perrault-Puss_in_Boots that referenced this issue Jul 9, 2014
@mkaschenko mkaschenko Make Tale::Miller use Tale::Inheritance 69af436
@dideler
dideler commented Oct 9, 2014

Something the guideline isn't clear on: What's the suggested way of describing a method that takes arguments? For example

def foo(bar)
  ...
end

describe "#foo(bar)"
# vs
describe "#foo"
@Spaceghost

@dideler When describing a class method, I use . and for an instance method, I use #.

I also include the method signature in the string like #foo(bar=:bar, baz={})

@dideler
dideler commented Oct 10, 2014

Thanks @Spaceghost

@adibsaad

Fyi :: is called the scope-resolution operator.

@diegoguemes

As some people already said, I think this syntax is amazing for describing methods, but it's not good enough for describing behaviors. It won't be useful for developers that are not familiar with the code.

Maybe I'm getting too far here, but I see it like redundant comments for getter and setters that don't add any additional value.

describe '#purchase' do
   # ...
   item.purchase
   # ...

Doesn't seem very DRY.

@dideler
dideler commented Oct 24, 2015

For reference, here's the guideline

BAD

describe 'the authenticate method for User' do
describe 'if the user is an admin' do

GOOD

describe '.authenticate' do
describe '#admin?' do

@diegoguemes

it's not good enough for describing behaviors

The behaviour is described by the inner blocks. E.g.

describe Item do
  describe '#purchase' do
    context 'when given a quantity' do
      it 'purchases n items'
    end
  end
end

I see it like redundant comments for getter and setters that don't add any additional value.

The naming convention provides consistency across tests and ensures related tests are grouped, so readers can easily navigate and find what they're looking for, and test output clearly shows what part of the system is under test.

Doesn't seem very DRY.

Be careful not to make your tests too DRY. Tests should be DAMP (Descriptive and Meaningful Phrases) which favours readability over anything else while still eliminating duplication where possible if it enhances readability.

@wakproductions

👍 @dideler right on

@jerzygangi

I fundamentally disagree with describing methods -- I write specs to describe behavior. For me, one of the most useful parts of RSpec is building a living documentation of what my application is doing -- in non-technical lingo. I intentionally do not refer to methods inside my it or describe or context descriptions. The result is a very easy to read, easy to scan, documentation of what my code is doing.

@jerzygangi

(And rspec --format documentation is great for this)

@mkaschenko

@jerzygangi could you provide an example?

@Spaceghost

One thing to consider is that there are different kinds of tests. Unit,
integration, and functional. I always describe behaviour, but at different
granularities depending on what kind of test it is.

There's definitely a lot of space for multiple distinct approaches
depending on the type of test you're writing. I write tests a bit more
similar to what I think you're describing, but those are functional tests
that express the inputs and outputs of the system, the parts the user
interacts with.

~Johnneylee

On Sat, Mar 19, 2016 at 9:19 PM, Maxim Kashchenko notifications@github.com
wrote:

@jerzygangi https://github.com/jerzygangi could you provide an example?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2 (comment)

@irisbune

@jerzygangi That is exactly what I find so confusing.

I'm learning RoR and RSpec now. In the bootcamp I joined, it was made clear that RSpec is meant to provide test output that is understandable to all, especially non-programmers. This GOOD vs BAD example does the exact opposite?

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