rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 356 forks source link

Kernel.warn when using Forwardable delegators with a null object #1441

Open bannable opened 2 years ago

bannable commented 2 years ago

Subject of the issue

When using Forwardable delegators, if the destination object is a spy, a kernel warning will be emitted of the form:

warning: [Class]#[method name] at [file]:[line] forwarding to private method RSpec::Mocks::InstanceVerifyingDouble#[method name]

Your environment

Steps to reproduce

require 'forwardable'
require 'rspec/core'
require 'rspec/mocks'

class ForwardableTest
  extend Forwardable
  def_delegator :@dest, :bar
  def initialize(dest)
    @dest = dest
  end

  def foo
    bar
  end
end

class Injected
  def bar; true; end
end

RSpec.describe 'fowardable behavior' do
  subject { ForwardableTest.new(thing).bar }

  context 'with an object instance' do
    let(:thing) { Injected.new }

    specify { expect { subject }.not_to output.to_stderr }
  end

  context 'with a null object (spy)' do
    let(:thing) { instance_spy('Injected') }

    specify { expect { subject }.not_to output.to_stderr }
  end
end

Expected behavior

Both examples will pass.

Actual behavior

fowardable behavior with a null object (spy) is expected not to output to stderr fails.

pirj commented 2 years ago

It seems that this:

  def foo
    bar
  end

can be removed, and the error still reproduces.

JonRowe commented 2 years ago

Adding an expected result solves this, such as:

instance_spy('Injected', bar: true)

So I think this is an artefact from the forward, as instance doubles only verify methods which are stubbed, where as the spy mode calls as_null_object where everything returns self, so is more like a method missing call, I think forward is complaining about that

pirj commented 2 years ago

By looking at the implementation, it doesn't look like Forwardable's warning is fair ([1], [2]). I'm not certain if it's Forwardable or instance_spy to blame.

I guess that spy is accepting and records every method call with method_missing. But does it define respond_to_missing? to declare that it does so? Is this sufficient for Forwardable to work and skip this warning? Are you interested in getting to the bottom of this?

I'd start with an attempt to reproduce the warning with a class defining dynamic methods used as a delegator target.

JonRowe commented 2 years ago

Our doubles respond_to? true for this case, it seems Forwardable checks theres an actual method with this warning, we don't define that unless you specifically mock out the method, I don't think we want to change that behaviour, and given there is an easy alternative, I'm inclined to close this as a "not something we can fix"