jimweirich / rspec-given

Given/When/Then keywords for RSpec Specifications
https://github.com/jimweirich/rspec-given
MIT License
653 stars 61 forks source link

Add optional description string to Then clause #17

Closed ronen closed 11 years ago

ronen commented 11 years ago

Hi, here's a small patch to allow explicitly specifying a description for a Then clause. Potentially useful for cases where the source code itself isn't particularly clear or compact for a report.

e.g.

Then("Widget form has a highlighted Undo button") {
   page.should have_selector("#Widget form button.active",  :text => "Undo")
}
jimweirich commented 11 years ago

I'm actively resisting adding comment strings to Then in order to promote better reading tests. I would suggest you are better off with a test that reads like this:

Then { page.should have_a_highlighted_button_named("Undo") }

The matcher is fairly easy to write (warning, untested code):

matcher :have_a_highlighted_button_named do |button_name|
  match do |page|
    page.should have_selector("#Widget form button.active",  :text => button_name)
  end
end
ronen commented 11 years ago

I do see your point. And while I agree that generally speaking readable code is better, the define-a-matcher approach means that you're writing test code which itself needs to be tested (is as you implicitly pointed out). This makes the testing process a bit more fragile and a bit more time consuming. Also it puts more lines of code in the test suite, and makes the test a bit less local. So for simple one-off tests it seems to me to be a bit excessive. It does encourage re-use though, which is a benefit when that 'one-off' test isn't as one-off as you expected. So as with all things programming, it seems to me that there's a balance to be struck.

As an intermediate, I've implemented this, to maintain the linguistic style and minimize the amount of verbiage needed for simple tests:

Where(:have_a_highlighted_undo_button) { page.should have_selector("#Widget form button.active", :text => "Undo") }
Then { should have_a_highlighted_undo_button }
Then { should_not have_a_highlighted_undo_button }

The 'Where' doesn't care about an actual, it evaluates in the example context. For simple reuse, it can also take parameters:

 Where(:have_a_highlighted_button) { |button_name| page.should have_selector("#Widget form button.active", :text => button_name) }
 Then { should have_a_highlighted_button "Undo" }

What do you think of that?

(BTW all this is a made-up example.)

Here's the implementation:

module GivenWhere
  def Where (symbol, &block)
    raise "Duplicate matcher definition #{symbol.inspect}" if RSpec::Matchers.instance_methods.include? symbol
    RSpec::Matchers.define symbol do |*args|
      match do |actual|
        matcher_execution_context.instance_exec *args, &block
      end
    end
  end
end
RSpec.configure do |config|
  config.extend GivenWhere
end

(Note the test for duplicate definition. Another problem with defining a matcher is that RSpec doesn't support local matcher definition. So all matchers must be defined globally. With a large application having multiple developers it could be possible for developers working on different parts of the system to define different matchers named :have_a_highlighted_button for the sake of high-level readability. And RSpec doesn't warn if you redefine the matcher, you'd just clobber the old one and mysteriously break previously-working tests.)