Closed tmertens closed 4 years ago
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?
@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.
Well the callback would probably work, although it is misnamed for this purpose...
Well the callback would probably work, although it is misnamed for this purpose...
How would the callback work here?
Well rspec-mocks
could call the callback when creating encountering an any_instance
call
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 :(.
Hence why I said it'd be misnamed, I have no objection to creating a second callback and repeating the implementation here...
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).
@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
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.
:+1:
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.
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. :)
Model.define_attribute_methods
would force rails to load methods without doing any db queries
This bit me today too. We're on rspec-rails 3.8.0
.
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
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?
This is still an issue as far as I can tell. The callback isn't even being called in this case because it's not a verified double but a partial double. Would be nice if rspec-mocks could add a callback to when the expectation is being created (or move the callback here) so it would apply in all cases.
I've used this patch and it seems to work:
module RSpec
module Mocks
class AnyInstanceAllowanceTarget < TargetBase
def initialize(klass)
super
if Class === klass && ActiveRecord::Base > klass && !klass.abstract_class?
klass.define_attribute_methods
end
end
end
end
end
@dorner sadly that patch is not something we can merge, as the gems are independent of each other, it is what we try to setup here: lib/rspec/rails/active_record.rb#L14 but something in that autodetection is not working correctly, I don't suppose you have some time to dig into the ordering issue there?
@JonRowe The issue is the when_declaring_verifying_double
callback. That's only called when using methods like object_double
or instance_double
. The callback isn't called at all when using any_instance
.
I suppose a better fix would be to add one more callback to rspec_mocks
that is triggered when an any_instance
is set up, which would basically mirror my ghetto patch above.
I suppose a better fix would be to add one more callback to rspec_mocks that is triggered when an any_instance is set up, which would basically mirror my ghetto patch above.
Or just make it use the existing one, the issue is we have to provide api's for rspec-rails to call.
Yeah exactly. I have a long to-do list right now but I can tack on adding the callback to rspec-mocks. Not sure when I'll have a chance to do it though. :( If anyone else wants to, it's probably a really small change.
We just need to trigger the existing one, I think
Closed by rspec/rspec-mocks#1309
Today I found this issue while using ActiveResource
.
Looking into the recommended way to stub dynamic classes attributes I found the following workaround.
ShopifyAPI::LineItem.define_method(:fulfillment_status) { super }
allow_any_instance_of(ShopifyAPI::LineItem).to receive(:fulfillment_status)
But it would be nice if ActiveResource
attributes were recognized the same way ActiveRecord
ones do. WDYT @JonRowe ?
@00dav00 AFAIK ActiveResource is no longer part of Rails and is an external gem, as such no configuration for it belongs in rspec-rails, you could of course release a gem for adding this, the config would be roughly:
::RSpec::Mocks.configuration.when_declaring_verifying_double do |possible_model|
target = possible_model.target
# I am assuming active resource does not have `abstract_class?` like active record does
if Class === target && ActiveResource::Base > target
# Something that defines methods based on a schema
end
end
To be included in a railtie or other configuration.
Of course that assumes that Active Resource knows about its methods like Active Record does... The implementation for ActiveRecord works because it's just lazy, the hook triggers a schema load (I think) which defines all the attributes you'd expect.
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
, andverify_partial_doubles
is enabled, rspec-rails will fail to resolve the dynamic method if it is stubbed viaany_instance
prior to any instance(s) of that class being instantiated.Environment
Steps To Reproduce:
rails g model Article title:string text:text
Article
in the stub toArticle.new.class
: