Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic method verification fails when method stubbed using allow_any_instance_of on a class not previously instantiated. #1357

Open
tmertens opened this issue Apr 23, 2015 · 17 comments

Comments

Projects
None yet
9 participants
@tmertens
Copy link

commented Apr 23, 2015

Dynamic method verification fails when method stubbed using allow_any_instance_of on a class not previously instantiated.

Given a test stubs a method on a class using allow_any_instance_of, and verify_partial_doubles is enabled, rspec-rails will fail to resolve the dynamic method if it is stubbed via any_instance prior to any instance(s) of that class being instantiated.

Environment

  • ruby 2.1.5
  • rails (3.2.19)
  • rspec-core (3.2.3)
  • rspec-expectations (3.2.1)
  • rspec-mocks (3.2.1)
  • rspec-rails (3.2.1)
  • rspec-support (3.2.2)

Steps To Reproduce:

  • Create a rails app
  • Generate a model with some fields: rails g model Article title:string text:text
  • Add the following test:
require 'rails_helper'

RSpec.describe 'dynamic methods' do
  it 'does not resolve dynamic instance methods on first call to any_instance' do
    allow_any_instance_of(Article).to receive(:title).and_return 'foo'
    a = Article.new
    expect(a.title).to eql 'foo'
  end
end
  • Migrate and run the test. It will fail with the message:
RSpec::Mocks::MockExpectationError: Article does not implement #title
./spec/models/any_instance_spec.rb:5:in `block (2 levels) in <top (required)>'
-e:1:in `load'
-e:1:in `<main>'
  • Change Article in the stub to Article.new.class:
  it 'does not resolve dynamic instance methods on first call to any_instance' do
    allow_any_instance_of(Article.new.class).to receive(:title).and_return 'foo'
    a = Article.new
    expect(a.title).to eql 'foo'
  end
  • Run the test again. It will pass.
@myronmarston

This comment has been minimized.

Copy link
Member

commented Apr 23, 2015

I believe this has to do with how activerecord works -- the dynamic column methods are not initially defined, so that Article.method_defined?(:title) lies and returns false. Our docs mention this gotcha:

https://relishapp.com/rspec/rspec-mocks/v/3-2/docs/verifying-doubles/dynamic-classes

However, @JonRowe added an improvement in #1238 that addresses this for instance_double. @JonRowe, do you think you can leverage that solution for this case, too?

@tmertens

This comment has been minimized.

Copy link
Author

commented Apr 23, 2015

@myronmarston This is true, but the rspec-rails documentation states that it resolves this: https://www.relishapp.com/rspec/rspec-rails/v/3-2/docs/model-specs/verified-doubles

In fact, it works perfectly for instance_double. The above test, rewritten as below, does not suffer from the same problem that any_instance does:

  it 'does not resolve dynamic instance methods on first call to any_instance' do
    instance_double('Article', title: 'foo')
    a = Article.new
    expect(a.title).to eql 'foo'
  end

It seems as you already stated that the solution for instance_double should be propagated to allow/expects on any_instance.

@JonRowe

This comment has been minimized.

Copy link
Member

commented Apr 26, 2015

Well the callback would probably work, although it is misnamed for this purpose...

@myronmarston

This comment has been minimized.

Copy link
Member

commented Apr 26, 2015

Well the callback would probably work, although it is misnamed for this purpose...

How would the callback work here?

@JonRowe

This comment has been minimized.

Copy link
Member

commented Apr 27, 2015

Well rspec-mocks could call the callback when creating encountering an any_instance call

@myronmarston

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

Well rspec-mocks could call the callback when creating encountering an any_instance call

The callback is named when_declaring_verifying_double and this isn't a verifying double, though. It's a partial double. Also, we don't have a module reference object for this case but I suppose we could make one. Doesn't seem ideal, but I don't have a better idea :(.

@JonRowe

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

Hence why I said it'd be misnamed, I have no objection to creating a second callback and repeating the implementation here...

@myronmarston

This comment has been minimized.

Copy link
Member

commented Apr 28, 2015

Hence why I said it'd be misnamed, I have no objection to creating a second callback and repeating the implementation here...

I'd rather not have two callbacks. Instead, can we come up with a better name for the callback that encompases both use cases? Then we can rename it (while keeping an alias for the old name for backwards compatibility to satisfy SemVer since we declared it public).

@tmertens

This comment has been minimized.

Copy link
Author

commented May 13, 2015

@JonRowe I tried running a test affected by this issue using the master branches of rspec containing your fix, and the issue persists:

%w[rspec-core rspec-expectations rspec-mocks rspec-support rspec-rails].each do |lib|
      gem lib, :git => "git://github.com/rspec/#{lib}.git", :branch => 'master'
    end
@tmertens

This comment has been minimized.

Copy link
Author

commented May 13, 2015

For reference, I'm using this monkey patch to work around the issue for now, in case anyone else runs into it:

require 'rspec/mocks'

# This is a workaround for https://github.com/rspec/rspec-rails/issues/1357
# until an official fix is available.
if defined?(ActiveRecord)
  module AnyInstanceFix
    def initialize(target)
      if (!target.is_a?(Class) || target != ::ActiveRecord::Base)
        target.define_attribute_methods if target.respond_to?(:define_attribute_methods)
      end
      super
    end
  end

  module RSpec
    module Mocks
      class TargetBase
        prepend AnyInstanceFix
      end
    end
  end
end

EDIT: Revised to not call define_attribute_methods when stubbing methods on the ActiveRecord::Base class itself.

EDIT2: Revised again to check target is a class to prevent corner case error in certain cases when calling != on target, which will probably never affect anyone but me :/. Also moved ActiveRecord defined? check since it only really needs to happen once.

@mhenrixon

This comment has been minimized.

Copy link

commented Nov 2, 2015

👍

@tomstuart

This comment has been minimized.

Copy link

commented Nov 28, 2017

As a data point: this bit me today during a project to enable partial double verification on an existing Rails codebase. Naturally a future project will be to eliminate uses of allow_any_instance_of, but I want to finish the current job first.

For now I’m going to edit the failing example to add Article.define_attribute_methods before the use of allow_any_instance_of(Article) and move on.

@caioeps

This comment has been minimized.

Copy link

commented Dec 22, 2017

This problem also hit me today.

I noticed that the test failed only if executed in isolation. So i decided to force the "loading" of the class by myself.

If you do encounter this kinda of problem with an ActiveRecord model, you should be able to do the following:

Model.create(args) # In my case I use FactoryBot for this
Model.all.first.method # Force rails to load methods
allow_any_instance_of(Model).to receive(:method)

My test is now passing. If I'm wrong in any point, please let me know. :)

@JonRowe

This comment has been minimized.

Copy link
Member

commented Dec 23, 2017

Model.define_attribute_methods would force rails to load methods without doing any db queries

@seanhandley

This comment has been minimized.

Copy link

commented Jul 12, 2019

This bit me today too. We're on rspec-rails 3.8.0.

@benoittgt

This comment has been minimized.

Copy link
Member

commented Jul 13, 2019

Does it mean we should rewrote the patch we avoid doing manually Model.define_attribute_methods? I can look at it. Feel free to assign it to me @JonRowe

@JonRowe

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Happy for you to tackle this @benoittgt, the relevant call to the callback lives in lib/rspec/rails/active_record.rb, I wonder if its not detecting the classes as models, does the inheritance check still work with the new include in later rails?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.