class spies fail to verify when chaining an instance method onto a class method that returns an instance

[imtayadeway]

Ran into this issue today, which I've reproduced approximately here:

class Foo
  def self.find
    new
  end

  def bar
    'bar'
  end
end

class Baz
  def qux
    Foo.find.bar
    'qux'
  end
end

RSpec.configure do |config|
  config.mock_with :rspec do |mocks|
    mocks.verify_partial_doubles = true
  end
end

RSpec.describe 'verifying doubles' do
  it 'raises an error when a chaining an instance method onto a class method that returns an instance' do
    foo_class = class_spy(Foo).as_stubbed_const
    expect(Baz.new.qux).to eq('qux')
  end
end

Basically I want to ignore some implementation detail of Baz by stubbing out Foo. I get the following error however on executing this test:

  1) verifying doubles raises an error when a chaining an instance method onto a class method that returns an instance
     Failure/Error: Foo.find.bar
       Foo does not implement: bar
     # ./verifying_doubles.rb:13:in `qux'
     # ./verifying_doubles.rb:27:in `block (2 levels) in <top (required)>'

Could this be an issue? Or could it be considered bad practice to use a class spy like this? Thanks!

[myronmarston]

Thanks for reporting this! As surprising as this behavior may be, it's actually working as it's meant to, although it's definitely confusing and we should do something about that. The problem has to do with how as_null_object works (which spies are implemented using). as_null_object essentially causes the test double to have every method stubbed to return itself, supporting arbitrarily deep method chains--like with a BlackHole object as provided by @avdi's naught. So, while the real implementation of Foo.find returns an instance of Foo, the Foo class spy you have created returns Foo itself from find, since you have not stubbed it to do anything else...and as a result, bar is being called on Foo (which doesn't have a bar method), not on a Foo instance spy.

For this to work as you've tried to use it, RSpec would have to know that the implementation of Foo.find returns an instance of Foo -- but RSpec obviously doesn't know that and never will.

I'm realizing that as_null_object has multiple facets of behavior that are not always desirable:

• It switches a test double from being "strict" (where every received message must be expected or allowed beforehand) to being "loose" (where it responds to every message without each being explicitly configured).
• It causes it to return self by default form any unrecognized message.

The latter is useful for "black hole" type objects, but can lead to confusing behavior in cases like these. I'm thinking maybe we should decouple these two behaviors (or at least provide a way to get the first without also pulling in the second) but I haven't thought far enough ahead to suggest what the API or semantics of that would be.

For your particular case, you can make it work by explicitly stubbing Foo.find to return an instance_double(Foo). Does that make sense?

[imtayadeway]

Thanks for your thoughtful words on this! I realized after submitting this that creating an issue may have been premature, because I do know that spies are null objects that behave as you indicated above, and as such should not have expected the above to behave any differently.

That said, I will agree that it could be confusing, and I was a little disoriented myself. My test passed when I wrote it because I was running it in isolation, and so the stubbed class never got loaded/verified. It was only later when I ran the full test suite that I got a failure, and it took a few passes to work out what had happened.

I'll be happy to dig into this some more if you think it's something that could be improved upon. Thanks!

[myronmarston]

One thing we can consider is allowing the user to configure how as_null_object behaves. Maybe an API like:

double(...).as_null_object(:return => nil) # each message will return nil
# or
double(...).as_null_object(:return => :self) # `self` will be returned from each message

That would allow you to have null objects that respond to every message but return nil by default instead of self, which could help prevent confusion. I'm not sure how to make the :return option fit into spy, though; spy already accepts a hash of message/return value pairs and I don't really want to make :return a "special" key that does something different in that hash.

[avdi]

On Tue, May 19, 2015 at 11:22 PM, Myron Marston notifications@github.com wrote:

I'm realizing that as_null_object has multiple facets of behavior that are not always desirable

And there you get at why I wound up making Naught a "null object toolkit" - it turns out null objects can have a number of different, orthogonal properties!

[cupakromer]

I'm not sure how to make the :return option fit into spy, though; spy already accepts a hash of message/return value pairs and I don't really want to make :return a "special" key that does something different in that hash.

Am I missing something on the spy syntax that is different from double? Won't your example also work for spys?

double("Widget").as_null_object(:return => :self)
double("Widget", :type => :sample).as_null_object(:return => nil)
spy("Widget").as_null_object(:return => :self)
spy("Widget", :type => :sample).as_null_object(:return => nil)

[myronmarston]

Am I missing something on the spy syntax that is different from double? Won't your example also work for spys?

The problem that spy(...) already applies as_null_object. spy literally is double.as_null_object. So it's a bit silly to do spy.as_null_object because you're calling as_null_object twice.

[imtayadeway]

I've thought some more about this and think I can identify what would make this a happy outcome for me. So, while I do like the suggestions above, what I would personally like is the ability to create a stubbed constant using RSpec's convenience method that does behave like a black hole null object, but without making it a class_spy, because in other cases I want to verify class spies, and in this I don't.

There is probably a deeper issue that I'm ignoring here, and it's an issue I've made for myself. It probably also extends beyond the scope of this discussion, but I'll summarize anyway. To give a little background: I"m reworking a Rails project's spec suite by dividing up the tests into unit/integration tests. The former require spec_helper, which loads nothing, and the latter require rails_helper, which loads the rails application. Individual unit tests require the file containing the SUT, and no more. The verifying double config was then placed into the rails helper, because the isolated tests could not verify anything without loading dependencies. So basically everything worked as I expected, and I got what I want out of class_spies as indiciated above, so long as I ran rspec spec/unit and rspec spec/integration separately. I just tripped myself up when I ran rspec spec/.

I'm happy to accept that the solution to my own problem is: don't do that. =)

[myronmarston]

So, while I do like the suggestions above, what I would personally like is the ability to create a stubbed constant using RSpec's convenience method that does behave like a black hole null object, but without making it a class_spy, because in other cases I want to verify class spies, and in this I don't.

You can do that by using stub_const directly:

stub_const("Foo", double.as_null_object)

The .as_stubbed_const method is just sugar for this, available when using a class double or class spy since we know the class name.

[imtayadeway]

@myronmarston thanks! so this has boiled down to my not reading the docs properly :blush:

i do like the idea of configurable null objects though, and happy to pick this up if the spy API can be mitigated.

[cupakromer]

The problem that spy(...) already applies as_null_object. spy literally is double.as_null_object. So it's a bit silly to do spy.as_null_object because you're calling as_null_object twice.

Dipping into the implementation details I'm not sure I see it that way. We internally made the spy a double.as_null_object, but that doesn't mean it would always have to be. Additionally, as_null_object already mutates the underlying double object. Both of these decisions keep the API clean.

A spy can simply be seen as a double which defaults to the "black hole" returning self. To get a different behavior we can use the same API call.

a_widget = double("Widget")
a_widget.as_null_object                      # This is now a "black hole" object
a_widget.as_null_object(:return => nil)      # It now always returns `nil`

a_widget_spy = spy("Widget")                 # A "black hole" object
a_widget_spy.as_null_object(:return => nil)  # Use the `nil` implementation

I can see this being useful, occasionally, with say specs of the form:

let(:current_project) { Project.new(an_activity) }
let(:an_activity) { spy("Activity") }

it "completing the project marks the activity as complete" do
  current_project.mark_complete
  expect(an_activity).to have_received(:terminate)
end

it "works when the activity doesn't have properties" do
  an_activity.as_null_object(:return => nil)

  current_project.mark_complete

  expect(an_activity).to have_received(:terminate)
end

[JonRowe]

Is there anything to do here? Seems not?

[myronmarston]

Is there anything to do here? Seems not?

Nothing specific to do to address @imtayadeway's issue, although it might be useful to provide a way to configure what our null object doubles return at some point.

[myronmarston]

A spy can simply be seen as a double which defaults to the "black hole" returning self. To get a different behavior we can use the same API call.

That would work, but it bothers me that in an expression like spy.as_null_object(:return => nil), spy doesn't do different than double even though it normally does.

[cupakromer]

That would work, but it bothers me that in an expression like spy.as_null_object(:return => nil), spy doesn't do different than double even though it normally does.

I can see that. As previously discussed spy is just sugar over double.as_null_object. In essence it really doesn't do much differently than double once as_null_object is called anyways.

[regishideki]

I have a problem with this spy behavior (return self when calling a non-existing method) because I have some verifications in my code that verifies if a method of an instance is nil. Example:

def foo
  return if @bar.baz.nil?

  do_something
end

In my test, I cant' use bar = spy('bar') because it will return false to bar.baz.nil? So, I need to do it explicit:

bar = double('bar', baz: nil)

That is a way to simple configure double or spy to return nil when calling a non-existing method? I think the implementation of as_null_object(return: nil) would solve the problem but it is not implemented yet, right?

[myronmarston]

In my test, I cant' use bar = spy('bar') because it will return false to bar.baz.nil? So, I need to do it explicit:

You can still use a spy; just pass the same baz: nil hash to spy instead of double:

bar = spy('bar', baz: nil)

Spies as null object doubles return self in response to any messages that have not been allowed, but you can allow baz to return nil on a spy just like you can on a normal double.

[regishideki]

@myronmarston I think I did not explain well (maybe I could use spy in both snippets). I know that. The concern here is that in my case there is no gain in using spy. And I thought would be great if it was possible to change the behavior of spy (or even double) when receiving a non-stubed method.

[JonRowe]

The advantage of using a spy is to set the method expectations after the methods have actually been received, e.g. it enables the have_received matcher, if you don't need that you can use a normal double which will not return anything by default, we need to return a double here to allow message chaining expectations for spies.