Inconsistency between if + else and if -> unless

Question

I had an issue today with defining custom RSpec matchers that I resolved, but couldn't actually see any reasoning behind why one of the approaches works and the other doesn't, here's the code:

Approach 1 -- if + else:

RSpec::Matchers.define :have_success_message do |message|
  match do |page|
    if message.nil?
      page.should have_selector('div.alert.alert-success')
    else
      page.should have_selector('div.alert.alert-success', text: message)
    end
  end
end

Approach 2 -- if followed by unless

RSpec::Matchers.define :have_success_message do |message|
  match do |page|
    page.should have_selector('div.alert.alert-success') if message.nil?
    page.should have_selector('div.alert.alert-success', text: message) unless message.nil?
  end
end

I figure the first approach is better because it checks the condition only once, but nevertheless, the result should be the same, right?

Well, it turns out the tests with the first approach pass, while the tests with the second don't. I am completely clueless as to why that is and would love if anyone could shed some light on that.

Edit:

Forgot to add the actual tests (with approach 2):

With the following HTML tag present:

<div class="alert alert-success">Profile updated</div>

I run 4 separate tests:

it { should have_success_message } # fails
it { should have_success_message('Profile updated') } # passes
it { should have_selector('div.alert.alert-success') } # passes
it { should have_selector('div.alert.alert-success', text: "Profile updated") } # passes

The failure is with following message:

1) User pages edit with valid information 
 Failure/Error: it { should have_success_message }
   expected #<Capybara::Session> to have success message
 # ./spec/requests/user_pages_spec.rb:80:in `block (5 levels) in <top (required)>'

When the HTML tag is not present, all 4 tests fail.

Edit 2:

I've tried another approach to verify whether control flow is correct:

Approach 3:

if message.nil?
  puts "In if, message is: #{message.inspect}"
  page.should(have_selector('div.alert.alert-success'))
end
unless message.nil?
  puts "In unless, message is: #{message.inspect}"
  page.should(have_selector('div.alert.alert-success', text: message))
end

With this approach the behavior is the same as with approach 2 - first test fails, following 3 pass.

Output is the following:

In if, message is: nil
In unless, message is: "Profile updated"

So the control flow looks alright, but

page.should(have_selector('div.alert.alert-success'))

fails, even though it passes outside the matcher. This truly is a mystery.

Final edit:

Just in response to the approved answer - when I switch the code like this:

page.should have_selector('div.alert.alert-success', text: message) unless message.nil? 
page.should have_selector('div.alert.alert-success') if message.nil?

The tests look like this:

it { should have_success_message } # passes
it { should have_success_message('Profile updated') } # fails
it { should have_selector('div.alert.alert-success') } # passes
it { should have_selector('div.alert.alert-success', text: "Profile updated") } # passes

So I think that indeed the last line, when it's not true, gets evaluated to nil, and that causes the whole mess. The first approach is better anyway, but I'm glad I have this problem out of my mind :)

Answer 1

This is correct behavior for RSpec, even though it seems unexpected.

Consider this code:

x = nil
"foo" if x.nil?
"bar" unless x.nil?
#=> 
"foo"
nil

The ...unless statement returns nil when the condition is falsey.

In your custom matcher, the ...unless statement returns nil when your message is nil.

That's the last line in your match block, so your match block returns nil.

Then RSpec sees your match block return nil, which RSpec considers the same as false, so RSpec reports your custom matcher failed.

Answer 2

Wow, that was a neat puzzle! The key is that the match method is expected to return a boolean result. In the first option, the implicit return value is the result of the if branch, which is true.

Why is it true? Well, should is defined like this:

      def should(matcher=nil, message=nil, &block)
        ::RSpec::Expectations::PositiveExpectationHandler.handle_matcher(self, matcher, message, &block)
      end

It delegates to PositiveExpectationHandler, whose handle_matcher method looks like this:

  def self.handle_matcher(actual, matcher, message=nil, &block)
    check_message(message)
    ::RSpec::Matchers.last_should = :should
    ::RSpec::Matchers.last_matcher = matcher
    return ::RSpec::Matchers::BuiltIn::PositiveOperatorMatcher.new(actual) if matcher.nil?

    match = matcher.matches?(actual, &block)
    return match if match

    message ||= matcher.respond_to?(:failure_message_for_should) ?
                matcher.failure_message_for_should :
                matcher.failure_message

    if matcher.respond_to?(:diffable?) && matcher.diffable?
      ::RSpec::Expectations.fail_with message, matcher.expected, matcher.actual
    else
      ::RSpec::Expectations.fail_with message
    end
  end

We can see that if the matcher returns true (or actually any truthy value), this is returned from the function and implicitly becomes the return value of should. I'm not sure if this behaviour is documented anywhere.

In the second option, however, the value of the last expression/statement wins, and because the unless condition is true, the expression evaluates to nil:

1.9.3-p0 :001 > x unless true
 => nil 

So RSpec thinks the matcher is trying to report failure because you're returning nil by accident.

---

To fix this, you probably should_not use should in the matcher. You can call through to Capybara's HaveSelector matcher like this:

if message.nil?
  have_selector('div.alert.alert-success').matches? page
else
  have_selector('div.alert.alert-success', text: message).matches? page
end

Incidentally, specifying text: nil eventually trickles through to here where it will result in the empty regexp, so the check for nil is not needed anyway and you can write the matcher like this:

match do |page|
  have_selector('div.alert.alert-success', text: message).matches? page
end

Not very Rubyesque, I'll admit, but there you go.