Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

`include` matcher is buggy when working with null_object test doubles #211

Closed
myronmarston opened this Issue Feb 15, 2013 · 9 comments

Comments

Projects
None yet
3 participants
Owner

myronmarston commented Feb 15, 2013

This fails but should pass:

require 'spec_helper'

describe "#include matcher" do
  describe "expect(...).to_not include(expected)" do
    context "for an array target" do
      it 'passes when given differing null doubles' do
        expect([double.as_null_object]).not_to include(double.as_null_object)
      end
    end
  end
end

The problem is the new functionality in #85. It considers a null object to be a matcher because it responds to matches?. The include matcher logic will always consider a null object to match, because null objects return self (which is truthy):

actual.any? { |value| expected.matches?(value) }

I'm not sure how best to fix this, but we need to solve it (or revert #85) before the next release.

@lukeredpath -- any ideas (since you originated the feature...)

@dchelimsky / @alindeman -- I'd love to hear ideas from you as well.

Owner

dchelimsky commented Feb 15, 2013

I've been thinking lately (not for the first time) that rspec-expectations should acquire rspec-mocks for 3.0. Relevant here only because then it would feel less (but only slightly) fowl to check that the type is not TestDouble (which is what I'd change Mock to if we were to merge the libs).

Another nasty alternative is: double.as_null_object.respond_to?(:matches?) #=> false, though I'm sure that would break somebody's test suite.

Two crappy ideas, and no good ones, but I'll stew on it.

Contributor

lukeredpath commented Feb 15, 2013

There's a strong possibility I'm being a bit dense here…but anyway. I would expect this to pass:
expect([double.as_null_object]).to include(double.as_null_object)
and this:
expect([double.as_null_object]).to == double.as_null_object
and this:
expect([double.as_null_object]).to match(double.as_null_object)
and the example you gave:
expect([double.as_null_object]).not_to include(double.as_null_object)
to fail, as it does.

My reasoning is that the object returned by double.as_null_object should essentially be a singleton or at the very least, have equal identity. I'd certainly expect them to match. What makes two different null objects of the same double different?

Why is this behaviour not correct? Like I said, I'm probably missing something obvious.

On 15 Feb 2013, at 05:52, Myron Marston notifications@github.com wrote:

This fails but should pass:

require 'spec_helper'

describe "#include matcher" do
describe "expect(...).to_not include(expected)" do
context "for an array target" do
it 'passes when given differing null doubles' do
expect([double.as_null_object]).not_to include(double.as_null_object)
end
end
end
end
The problem is the new functionality in #85. It considers a null object to be a matcher because it responds to matches?. The include matcher logic will always consider a null object to match, because null objects return self (which is truthy):

actual.any? { |value| expected.matches?(value) }
I'm not sure how best to fix this, but we need to solve it (or revert #85) before the next release.

@lukeredpath -- any ideas (since you originated the feature...)

@dchelimsky / @alindeman -- I'd love to hear ideas from you as well.


Reply to this email directly or view it on GitHub.

Owner

dchelimsky commented Feb 15, 2013

@lukeredpath would you expect this to pass or fail?

foo = double.as_null_object
foo.stub(:a => "a")

bar = double.as_null_object
bar.stub(:a => "A")

expect([foo]).to include bar
Contributor

lukeredpath commented Feb 15, 2013

I think I'd expect it to pass, i.e. I'm not convinced that stubs should change the objects identity.

What about this:

user_a = User.find(1) # active record
user_a.stub(:name => 'Joe')

user_b = User.find(1)
user_b.stub(:name => 'Tom')

expect([user_a]).to include user_b

I think that should pass.

On 15 Feb 2013, at 10:46, David Chelimsky notifications@github.com wrote:

@lukeredpath would you expect this to pass or fail?

foo = double.as_null_object
foo.stub(:a => "a")

bar = double.as_null_object
bar.stub(:a => "A")

expect([foo]).to include bar

Reply to this email directly or view it on GitHub.

Owner

dchelimsky commented Feb 15, 2013

I agree that that example should pass, but I think this should fail:

What about this:

user_a = User.find(1) # active record
user_a.stub(:name => 'Joe')

user_b = User.find(2)
user_b.stub(:name => 'Tom')

expect([user_a]).to include user_b

If that should fail, then I think this should as well:

user_1 = stub(:user, :id => 1).as_null_object
user_2 = stub(:user, :id => 2).as_null_object

expect([user_1]).to include user_2

The problem is we'd have know that :id is meaningful for that to work.

This all makes me think there's a missing concept here. DDD prescribes values, entities and services. IIRC value equality is based on value, entity equality is based on an identity attribute (e.g. id), and service equality is based on memory location. What about adding (or releasing in a separate lib) stub_entity? We shouldn't need a stub_value because you should be able to just make a real one, and the current stub could serve as a stub_service. Then you could say:

person_1 = stub_entity(:person, :id, :id => 1)
person_2 = stub_entity(:person, :id, :id => 2)
expect([person_1]).to_not include person_2

This would create objects that respond to == by comparing the values of the attribute named by the 2nd arg (:id in the example above).

Alternatively we could add a identified_by method to Mock e.g.

person_1 = stub(:person, :id => 1).identified_by(:id)
person_2 = stub(:person, :id => 2).identified_by(:id)
expect([person_1]).to_not include person_2

I'm sure there are other ways to tackle this, but basically we'd be saying that stubs are, by default, considered equal unless we declare otherwise. Thoughts?

Owner

myronmarston commented Feb 15, 2013

To make this more concrete...here's the spec in my app that failed when I updated to the latest rspec-expectations:

  describe Worker do
    let(:queue)   { fire_double("Qless::Queue", name: "shard", pop: nil).as_null_object }
    let(:queue_2) { fire_double("Qless::Queue", name: "scheduler", pop: nil).as_null_object }

    it 'excludes queues that have been toggled off via the config setting' do
      expect(Vanguard.settings.disabled_queues).to be_an(Array)
      Vanguard.settings.stub(:disabled_queues) { [queue.name] }

      expect(processed_queues).not_to include(queue)
    end 
  end

(There's more context here like the processed_queues method that I'm not showing for brevity).

This worked before, so I would consider this a regression, and regardless of what the "right behavior" is, this has the potential to break rspec suites in the wild.

All that said: I think the "right behavior" is for two test doubles to not be considered identical. Consider that test doubles, on their own, are a blank slate that can stand in for anything. There's no logical reason (in my mind) that they should be considered identical...if you've declared two separate test double objects, I don't see any reason to assume you are making them stand in for the same thing. In fact, you may be making them stand in for completely different types.

Note that the bug is only with doubles declared with as_null_object -- doubles without this work as they always have in the include matcher. as_null_object makes the double duck-type to a matcher. I don't think as_null_object should change an object's identity, though.

I was mulling on this last night, and here's one possible solution I came up with:

Change this:

def is_a_matcher?(object)
  object.respond_to?(:matches?)
end

to:

def is_a_matcher?(object)
  object.respond_to?(:matches?) && !is_a_null_object_test_double?(object)
end

def is_a_null_object_test_double?(object)
  object.respond_to?(:actually_im_not_a_matcher_i_respond_to_anything) &&
  object.actually_im_not_a_matcher_i_respond_to_anything.equal?(object)
end

I think that actually_im_not_a_matcher_i_respond_to_anything is a long/obscure enough method name that only something handling method_missing would respond to it...and as an additional check, we call it to verify that the method returns self. I haven't looked at other mocking frameworks (e.g. flexmock, mocha or rr) recently, but assuming they have a similar concept of a null object test double, this should work for those objects as well.

Thoughts?

Owner

dchelimsky commented Feb 15, 2013

I like the idea, and like the obscurity of the name, but don't like the specific name because it doesn't make sense to say actually_im_not_a_matcher_i_respond_to_anything.equal?(object) (to me). How about just rspec_null_object_test_double? e.g.

def is_a_matcher?(object)
  object.respond_to?(:matches?) && !rspec_null_object_test_double?(object)
end

def rspec_null_object_test_double?(object)
  object.respond_to?(:rspec_null_object_test_double?) &&
  object.rspec_null_object_test_double?
end

In fact, that could probably be even simpler:

def is_a_matcher?(object)
  object.respond_to?(:matches?) && !object.respond_to?(:rspec_null_object_test_double?)
end

I don't think we need to worry that anybody is going to break this behavior by stubbing rspec_null_object_test_double?, and if you're really worried about it the stub declaration could always raise an error:

obj = double(:evil, :rspec_null_object_test_double? => false)
#=> ArgumentError: you can't stub that method, silly. WTF are you trying to do, anyway?
Owner

myronmarston commented Feb 15, 2013

I was thinking that this part is important:

object.whatever_the_method_is.equal?(object)

The reason being, there are other objects out there that override respond_to? and method_missing to respond to everything...but a null object test double also has the property that it returns self so that you can send it method chains.

All that said, it occurs to me now that what we really care about are objects that duck type to the matcher protocol (e.g. by responding to matches?) but that aren't actually matchers -- and I have a hard time imagining anyone implementing a matcher that responds to everything -- so I think a respond_to? check or an obscure method name is sufficient.

Anyhow, as to which specific method name we use: the method name you suggested seems a little rspec-mocks specific -- while I would expect a mocha/rr/flexmock null object test double to pass this test as well, it seems a bit mis-named for those cases -- so I might stick closer to my original method name idea but not bother with calling the method and doing the equal check.

Hmm....one final thought on this: it occurs to me that matches? is a general enough name, that some folks may have domain objects that are not rspec matches that respond to this. This change has the potential to break specs in the wild for objects like that. So maybe we should check more than just matches? (even though we'll only call matches? here). @dchelimsky -- is there one other method name we can check that it responds to to very it's a matcher? Maybe failure_message_for_should ? Or is that not guaranteed to be on all matchers?

Owner

dchelimsky commented Feb 15, 2013

Definitely not guaranteed to be on all matchers in the wild because the matcher API has been around a long time and gone through some changes.

Might want to consider tightening this up as part of rspec-3, by choosing the API we want and deprecating the older APIs, but still supporting them through the rspec-3 cycle (i.e. removing them in rspec-4) and offering end-users a way to silence those deprecations while we wait for lib authors to catch up.

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