Spies create false positives when used with dynamic matchers

[cpetschnig]

The following spec is green:

require 'rspec'

RSpec.describe 'too nice spy' do
  example do
    spy_obj = spy('spy Object')
    expect(spy_obj).to have_receivedddddd(:foo) # passes
  end
end

It took me quite some time to figure out that it was actually my mistake, after I debugged deeply into RSpec internals. I would have expected an 'undefined method' error with my typo, but spies are very forgiving in such a case.

I don't know if this could be fixed in a reasonable way. If you think this is solely my concern and shouldn't be handled by RSpec, that's fine. Please then take my issue report simply as user feedback 😃 Thank you for your great work on RSpec!

[myronmarston]

Thanks for reporting this! That's a tricky one, because rspec-expectations supports dynamic matchers like have_foo(:bar) (which pass if subject.has_foo?(:bar) returns a truthy value). Since spies are null objects and return themselves when any message is sent, spy.has_foo?(:bar) returns a truthy value.

I think a fix for this will require some changes to the dynamic predicate matchers. I'm not sure what the right change is yet, though; one complication is that while your example would ideally fail, this should pass:

spy_obj = spy('spy Object', has_receivedddddd?: true)
expect(spy_obj).to have_receivedddddd(:foo)

IOW, if a specific predicate method has been stubbed on the spy, and you use the dynamic predicate matcher that corresponds to it--then it should pass. But if the method hasn't been specifically stubbed, we'd like it to fail. Problem is, in both cases the spy returns a truthy value.

So I'm not sure what the right fix, if any, is yet.

[cpetschnig]

After further thoughts on this, maybe expect(spy_obj) could implicitly disable the spying mode of the object, converting it in fact to a normal double. With an explicit call, the user could still turn it back to normal spy mode. What do you think about this approach?

I first encountered RSpec spies only a few weeks ago, but I can recall many situations in the past, were a spy would have been extremely useful. The have definitely become part of my Ruby/TDD toolset.

[myronmarston]

After further thoughts on this, maybe expect(spy_obj) could implicitly disable the spying mode of the object, converting it in fact to a normal double. With an explicit call, the user could still turn it back to normal spy mode. What do you think about this approach?

That's an interesting idea. I don't think it makes sense to toggle which kind of double it is, but expect(spy_obj) could restrict which matchers it works with, or warn if certain matchers were used.

[JonRowe]

We could fix this with an additional check in the dynamic matcher, checking it's not a spy.

[fables-tales]

We could fix this with an additional check in the dynamic matcher, checking it's not a spy.

👍

[cpetschnig]

An approach like this (in rspec-expectations)?

diff --git a/lib/rspec/matchers.rb b/lib/rspec/matchers.rb
@@ -958,6 +958,7 @@ module RSpec
     DYNAMIC_MATCHER_REGEX = Regexp.union(BE_PREDICATE_REGEX, HAS_REGEX)

     def method_missing(method, *args, &block)
+      return super if self.is_a?(RSpec::Mocks::Double) && null_object?
       case method.to_s
       when BE_PREDICATE_REGEX
         BuiltIn::BePredicate.new(method, *args, &block)

I haven't run this, but would make a PR if you approve. There might also be a RSpec.const_defined?('Mocks') necessary. It could also be made more extensible by adding a configuration option.

[JonRowe]

I'd support this, you'd also need to check RSpec::Mocks is defined of course

[nevinera]

At some point in the last 6 years, the example in question stopped being relevant because

expected #<Double "thing">.has_recieveddddddd?(:foo) to return false, got #<Double "thing">

it's now checking for true, instead of a truthy value. Is there still anything left here to do? The proposed solution still seems viable, but it's not clear how much value remains.

[pirj]

Good point, thanks for bringing this up.

It’s the ‘strict_predicate_matchers’ option introduced here that puts the matcher name typo to a light.

But this option is false by default, and specs still pass with it turned off with a mistyped matcher. It will be true by default in RSpec 4. But it’s still a configuration option and some may prefer to opt out. For them, a typo in have_received would result in a deceptive evergreen spec.

What options do we have? Remove the option and turn it on by default in RSpec 4? Still add a check if a dynamic predicate matcher was used with a spy? Issue a warning, or fail? @JonRowe what do you think?

[nevinera]

Ah, that explains it! Well, since it's still useful for now (and I expect rspec3 will be alive for a decade in some shops), I'm happy to throw together a PR. Back in a bit :-)

[nevinera]

We could fix this with an additional check in the dynamic matcher, checking it's not a spy.

I'm running into an awkwardness here that I didn't expect - the method_missing call here doesn't have access to the matched object; it's just creating a matcher, which will then receive that object.

I'm thinking I should update predicate_accessible? to have special behavior for the Double case, and return false if defined?(RSpec::Mocks::Double) && actual.is_a?(RSpec::Mocks::Double) && actual.methods.include?(predicate). I'll toss that up and see how it looks

[nevinera]

How's https://github.com/rspec/rspec-expectations/pull/1455 look?