be_within no longer understands == or === #161

Closed
jfarmer opened this Issue Jul 25, 2012 · 10 comments

Comments

Projects
None yet
4 participants

jfarmer commented Jul 25, 2012

As of 2.11.0, RSpec::Matchers::BuiltIn::BeWithin no longer has BaseMatcher in its call chain.

BaseMatcher defines:

module RSpec
  module Matchers
    module BuiltIn
      class BaseMatcher
        ...
        def ==(other)
          matches?(other)
        end
        ...
      end
    end
  end
end

So, since 2.11.0, be_within has been comparing using object identity rather than RSpec matching.

Now consider RSpec::Matchers::BuiltIn::Change:

class Change
  ...
  def expected_matches_actual?(expected, actual)
    expected === actual
  end
  ...
end

Womp womp. === falls back to == if it's not specifically defined, so now code like this doesn't work:

it "should work" do
   time = nil
   expect {
     time = Time.now
   }.to change{time}.from(nil).to be_within(0.1).of(Time.now)
end

This is because Change calls ===, which now in 2.11 falls back to the default Object ===.

A simple fix, which doesn't break any specs:

module RSpec
  module Matchers
    module BuiltIn
      class BeWithin
        ...
        def matches?(actual)
          @actual = actual
          raise needs_expected     unless defined? @expected 
          raise needs_subtractable unless @actual.respond_to? :-
          (@actual - @expected).abs <= @delta
        end

        alias_method :===, :matches?
        ...
      end
    end
  end
end

RSpec 2.10 defined == rather than ===, which seems "less good" to me. I don't know the code well enough to understand the implications one way or another. None of specs fail if I define BeWithin#== or BeWithin#===.

I don't have strong sense of RSpec's style/philosophy, so the above should be plenty to put in place a proper fix. I don't have time to write a test against a regression.

Owner

myronmarston commented Jul 25, 2012

Thanks for reporting this. I never thought of composing the matchers like this, but I can see how useful that is.

I need to figure out if all matchers should define == or === to delegate to matches?.

@dchelimsky -- can you shed some light here? Is == supposed to be defined on all built-in matchers?

jfarmer commented Jul 25, 2012

Woohoo, an accidental feature! :D

If you're free to choose between == and ===, I would definitely choose === and define it thus:

alias :case_equals? :===
def ===(actual)
  self.case_equals?(actual) || matches?(actual)
end

I'd piggy back on the already-defined === so that case statements still work as expected. It makes sense to put it in some abstract class so this behavior applies "for free" to all matchers.

I'd use === because == should be symmetric, i.e., a == b implies b == a for any two Ruby objects. If you define == in a way that's no longer symmetric, it will definitely introduce bugs because people assume (unconsciously) it is symmetric.

That is, someone, somewhere, will write actual == expected where they unknowingly needed to write expected == actual. I'd be surprised if there weren't code deep in the bowels of Ruby that assume it, too.

See: http://en.wikipedia.org/wiki/Symmetric_relation

On that note, I don't like that BaseMatcher#== is re-defined to be asymmetric. IMO that should be changed if possible.

The fact that === isn't symmetric weirds me out, but at least it's consistent with the rest of Ruby. For example

String === 'apple' # => true

but

'apple' === String # => false

So an asymmetric === is par for the course and anyone using it will go in assuming that possibility.

jfarmer commented Jul 25, 2012

BTW, this was the spec in our app that first alerted me to the change:

it "updates the last_emailed_at timestamp" do
  expect {
    user.emailed!
  }.to change{user.last_emailed_at}.from(nil).to be_within(1.second).of(Time.zone.now)
end
Owner

dchelimsky commented Jul 26, 2012

Is == supposed to be defined on all built-in matchers?

Yes, but the motivation was always composability within message expectations. No reason not to extend the same courtesy to other matchers :)

jfarmer commented Jul 26, 2012

Well, it's icky to me that == is now asymmetric in RSpec land and nowhere else in Ruby land, but if that's how it always has been then no need to break backwards compatibility.

Owner

myronmarston commented Jul 27, 2012

I just pushed a fix. As part of this I discovered lots of other built-in matchers that lacked ==.

Well, it's icky to me that == is now asymmetric in RSpec land and nowhere else in Ruby land, but if that's how it always has been then no need to break backwards compatibility.

This is how I feel as well--I'd prefer it was === but I figured it's best to maintain backwards compatibility here.

I was also hit by this, thanks for reporting and fixing.

Is this fix going to rubygems anytime soon?

Owner

myronmarston commented Aug 29, 2012

I'll try to get a release out soon. I'm traveling tomorrow, though, so it'll be at least a few days.

myronmarston added a commit that referenced this issue Sep 5, 2012

Ensure #== is defined on build in matchers so that they can be composed.
For example:

expect {
  user.emailed!
}.to change{user.last_emailed_at}.from(nil).to be_within(1.second).of(Time.zone.now)

Closes #161.
Owner

myronmarston commented Sep 5, 2012

@dgilperez -- I just released 2.11.3 with this fix.

@myronmarston thanks for the heads up!

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