Skip to content

Instantly share code, notes, and snippets.

@dhh
Created March 29, 2011 19:09
Show Gist options
  • Star 34 You must be signed in to star a gist
  • Fork 3 You must be signed in to fork a gist
  • Save dhh/893027 to your computer and use it in GitHub Desktop.
Save dhh/893027 to your computer and use it in GitHub Desktop.
describe "GET current" do
before do
@request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
get :current, :format => 'js'
end
it { should respond_with(:success) }
it { should set_cookie(:hidden_notices).to("#{notices(:permanent).id}") }
it { should render_template('notices/current') }
end
end
# vs
test "GET current (or preferably an explanation WHY we are testing it)" do
@request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
get :current, :format => 'js'
assert respond_with(:success)
assert_equal "#{notices(:permanent).id}", cookies[:hidden_notices]
assert_template 'notices/current'
end
@jrwest
Copy link

jrwest commented Mar 29, 2011

These aren't exactly the same thing, the example is a bit flawed. 2 options, in the RSpec example you could do the exact same thing with one "it" block and multiple shoulds (like your using one test block and multiple asserts) or you need to change the test/unit example to be 3 test cases. To be more accurate make the number of test cases and assertions per case the same. Then the examples look relatively similar and it boils down to do you prefer 'assert something' or 'object.should be_true'. I still prefer mini_test (test/unit) but I used to write a lot of my tests with RSpec.

describe "GET current" do
  it "does the same thing as your example" do
    @request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
    r = get :current, :format => 'js'

    r.should respond_with(:success) }
    r.should set_cookie(:hidden_notices).to("#{notices(:permanent).id}") }
    r.should render_template('notices/current') }
  end
end

@justinko
Copy link

In the T/U example, if assert respond_with(:success) fails, the two assertions that come after it will not run. This is not the case with the RSpec example.

@jrwest is correct - they are not the same.

@wilkerlucio
Copy link

quick question, test unit has some before/after related commands?

@christianromney
Copy link

A lot of it has to do with taste. For me, language is very important. something.should equal(:something_else) sounds a hell of a lot more natural where assert_equals something, :something_else sounds very robotic and requires me to translate from computer language to human language, however briefly. Now, I also think that more complete examples have a tendency to spin out of control, especially with nested describe blocks, and before blocks thrown in. (Have a look at generated specs for controllers - UGH).

@justinko
Copy link

@xmlblog - It really is about taste.

ActiveRecord & Datamapper
Prototype & JQuery
Postgresql & mysql
Vim & emacs
And much much more

Why do people pick on RSpec?

@lucianosousa
Copy link

confused comparing.

tdd is not equal bdd.

@jrwest
Copy link

jrwest commented Mar 30, 2011

Personally, I switched from RSpec back to Test/Unit (MiniTest) for 3 Reasons:

  • It is true that when you start nesting more and more describe/context blocks, specs become hard to read even with code folding. I started to prefer long method names describing context than trying to remember what sub-sub-sub-sub context I was in 500 lines into a spec file.
  • I was working on a project that was using mini test and I realized I was faster writing complex expressions and using simple assertions (assert, assert_true) than trying to remember all the really cool rspec matchers or being tempted to go on a tangent and write my own.
  • I really like the assert_performance_* class of assertions provided by mini_test

The change to MiniTest was key in my switch back especially because I could use the RSpec DSL a bit while I was switching over. I still will use and do use RSpec from time to time if let's say a project is already using it or someone asks me to.

@p8
Copy link

p8 commented Mar 30, 2011

There's one 'end' too much in the rspec example.

If you'd use Cucumber instead of a controller test, it would be clear for other developers why you're setting the cookie.
Scenario: Refreshing my notices
Given I have some notices
When I refresh my notices
Then I should only see my permanent notices on some page.

This will also make sure the correct keys are used for both getting and setting :hidden_notices

@p8
Copy link

p8 commented Mar 30, 2011

If you're advocating using classes for contexts shouldn't it be:

class GetCurrentForSomeControllerTest < ActiveSupport::TestCase
  def setup
    @request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
    get :current, :format => 'js'
  end

  test "responds with success" do
    assert respond_with(:success)
  end

  test "sets cookies because when I am on some page I only need to see my permanent notices" do
    assert_equal "#{notices(:permanent).id}", cookies[:hidden_notices]
  end

  test "renders the notices template because..." do
    assert_template 'notices/current'
  end
end

@eparreno
Copy link

"Opinionated" example. As we've already seen you can do the same with RSpec.
IMHO Some people loves sugar and some not, and this is not bad. Use tools that you feel comfortable with

@jacaetevha
Copy link

@justinko the only reason your statement is true is because of the way @dhh wrote the example. With the "shoulds" in a single "it" block the last 2 "should" statements will not run if the first fails, just as in T/U. @dhh is either being disingenuous or ignorant in this example -- take your pick.

@rubypanther
Copy link

rspec has more lolcats, surely that is worth some points?

@zerothabhishek
Copy link

As a newcomer to ruby/rails testing, I found Test:Unit much easier to get started with. Switched to rspec recently when was asked to, and have a feeling it fits more smoothly into the BDD workflow. Still, Test::Unit is surely not un-cool!

@btakita
Copy link

btakita commented Apr 1, 2011

Why have controller specs/tests? Testing on Rack is so much cleaner and easier to maintain.
I (nor the user) don't care what template got rendered, I just care what content got rendered.

Here's how I would really want to test this:

describe "GET /notices/current" do
  it "removes the prefix from the hidden_notices cookie and renders the current notices" do
    request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
    get "/notices/current", {}, {"HTTP_ACCEPT" => "text/javascript"}

    response.code.should == 200
    cookies[:hidden_notices].should == "#{notices(:permanent).id}"
    response.body.should include("something I actually care about")
  end    
end

@christianromney
Copy link

christianromney commented Apr 1, 2011 via email

@justinko
Copy link

justinko commented Apr 1, 2011

@btakita - "removes the prefix from the hidden_notices cookie and renders the current notices" <<- Isn't that a little much to digest? What if you needed to add another "should" in the example? The example description would get even more out of control.

IMO, one expectation per example really keeps things maintainable.

@btakita
Copy link

btakita commented Apr 2, 2011

@xmlblog - Sometimes "brittle" tests are ok and part of the normal development feedback loop. A lot of the time the content rarely changes so it's not brittle anyways. If it is brittle, then the test can change. I guess it can be annoying at times, but whatevs.
Sure I can accept that you want to know what template gets rendered. I actually like the convention where templates render a div with a custom html attribute which contains the template path. It's easy to tell what templates are rendered when viewing the page in the browser.

@justinko - Yeah, there's multiple pieces of behavior there. Splitting things up works out well in certain situations. Sometimes it's easier just to have it all in one spec. Of course personal taste plays into this.

@mattwynne
Copy link

Just read the two versions out loud, they speak for themselves. See also rspec foo.rb -f s

I'm surprised you don't get it, @dhh, you've always been a wonderfully big advocate of communicative code.

@rubypanther
Copy link

@mattwynne: if when confronted with differing opinions you assume the other party simply doesn't "get it" you prevent yourself from useful communication entirely.

Also, the read out loud thing makes a lot of implicit assumptions not related to "communicative code." Communicative code does not automatically mean that when it's spoken it will communicate well. It usually means when it's read it will communicate well. Punctuation in code is not easily read, which is one of many reasons why code is usually read, not spoken. While it is certainly possible to write code that is fluidly spoken, this is not any sort of requirement of clear code. Indeed in Ruby there is lots of punctuation!

oh_noes! unless @ruby.has_punctuation?

@mattwynne
Copy link

@rubypanther you're right, it was off-hand of me to say "get it". What I meant was "appreciate why people find this a useful technique". RSpec isn't The Silver Bullet, but if it's working for a significant number of people, I think it's healthy for us to be curious about why that's the case.

Personally, I really find that whether I'm reading code out loud or in my head, the closer it is to the way I'd describe the behaviour in English, the less likely I am to miss a mistake.

@rubypanther
Copy link

@mattwaynne: I don't think it's a matter of word choice at all. You're still replacing his opinion with lack of knowledge.

He's been quite clear about it. In fact, that's largely his point: it's heavily used because of a cargo cult. The question you'd have him ask is exactly the question to which his answer brought us all here.

@igbanam
Copy link

igbanam commented Jul 28, 2011

@jrwest there is no need for the r variable.

describe "GET current" do
  it "does the same thing as your example" do
    @request.cookies['hidden_notices'] = "1,#{notices(:permanent).id}"
    get :current, :format => 'js'

    response.should respond_with(:success) }
    response.should set_cookie(:hidden_notices).to("#{notices(:permanent).id}") }
    response.should render_template('notices/current') }
  end
end

But in the end still it comes down to taste.

@justinko
Copy link

@StevenGerrard

You have great code ... I love it

Thanks bro!

@cameron-martin
Copy link

I can't take a design opinion seriously from the person who built rails.

Plus all the things that other people have pointed out about these examples being no way near equivalent.

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