expect_any_instance_of when ::new called in method

[Marlin Pierce]

I saw the official discouragement of using expect_any_instance_of and allow_any_instance_of on the page:

https://relishapp.com/rspec/rspec-mocks/docs/working-with-legacy-code/any-instance

So, what should I do when I have a method which calls new to get a new object of a class,

and then calls a method on that new object?  I want to stub the method call, 

because it does some complicated things which external resources,

and I want to have an expectation that the method is called.

  def self.create_from_vendor(epoch, vendor)
    new(vendor).create(epoch)
  end

Is there a way I need to rewrite my code to test this better?

Is this a smell?

[Aaron Kromer]

It really depends on the objects. If that were PORO code I would pass in mocks for epoch and vendor. I would set my expectations of command methods on epoch and vendor accordingly as tests that the proper things are done. I would also check the public state of the returned object to ensure it was what I expected.

Now I’m guessing by the naming this is ActiveRecord related. In that case, if epoch and vendor are other AR objects, I would likely let this method hit the database. I would then set my expectations on the public API of the returned object (as above), I would also check the state in the database, and the state of the collaborators if applicable.

Now, when using this method, in say a controller spec, I would allow myself to stub it out (since it is your API): allow(ThatObjectClass).to receive(:create_from_vendor).and_return(this_mock_or_built_model).

​

--
You received this message because you are subscribed to the Google Groups "rspec" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rspec+un...@googlegroups.com.
To post to this group, send email to rs...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/rspec/60034cee-8d5d-4053-ba10-1816a6c4ef03%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Aaron Kromer]

I’ll just follow that up briefly to address your question of code smell. I would not use any_instance here. The reason being is that you want to test that this method does what you expect. While you are testing a class method, new is a method on that class object. This means you are stubbing the object under test. That is always (to me) a huge code smell.

In a nutshell, stubbing the object under tests makes your test essentially useless. I could incorrectly change the implementation of that method and your test will still happily pass. That’s not something you ever want.

If you are concerned about duplicating specs, I suggest taking a look at shared_examples which would allow you to bundle tests of say “does some general group of stuff when created with a vendor”. Then you could use them when testing that method and when testing create separately.

​

[Marlin Pierce]

I'm not sure if I quite get what you are saying.

The example code is a re-write from trying to avoid calling any_instance.  Maybe this will be clearer with the original code below.

def create_accounts(epoch, start_time)
  vendors = Vendor.new_vendors(start_time)
  vendors.each do |vendor|
    ProspectAccount.new(vendor).create(epoch)
  end
end

What I was trying to test was that in create_accounts, if Vendor.new_vendors returns a collection of some vendor objects, the create_accounts will call the ProspectAccount#create method.

To avoid using any_instance, I rewrote the call inside the loop to ProspectAccount.create_from_vendor(epoch, vendor), but that only moves the problem to the create_from_vendor method.  It does help that when testing the create_from_vendor method, I will only be dealing with one vendor, instead of looping through a collection of vendors.

The create method actually does some complicated stuff.  It writes data to a Redis database, and then makes a web service call to a queuing system.  What create actually should do is tested in other tests.

What I need to test is that create_accounts calls the ProspectAccount#create method.

I don't think your response helps me test that create is called, while stubbing out the already tested and complicated implementation of the create method.

I am gleaming from your response is that you would test that what happens in create would be tested in the create_accounts test, rather than testing that ProspectAccount#create is called.  That seems to me that you would test the functionality of the ProspectAccount#create method in the create_accounts test.  Then do you test the functionality of the create_accounts method in the test of the method which calls create_accounts?

BTW, The create is not the ActiveRecord create.  Also we are not using Rails, just ruby.  We don't have a view.  The closest we come to a controller object/class is the class that the create_accounts method is in.  Say for the sake of argument, that that class is named BatchController.

[Myron Marston]

Thanks Marlin, that’s a useful clarification.

Viewing any_instance as a code smell, and allowing that to put design pressure towards introducing the ProspectAccount.create_from_vendor(epoch, vendor) interface is a good thing, IMO. Consider that in ProspectAcount.new(vendor).create(epoch), the ProspectAccount instance is a throw away instance. It doesn’t serve any purpose other than to provide access to the create method. Given that one of the use cases ProspectAccount is to create from a vendor and account, why not make that use case more first class by explicitly providing a simple interface for that in the form of ProspectAccount.create_from_vendor(epoch, vendor)? Now you’ve got an interface that supports a simpler communication pattern and makes a main use case more obvious by providing an explicit facade for performing it.

Once you’ve provided ProspectAccount.create_from_vendor(epoch, vendor), it becomes far simpler to mock or stub, as Aaron said. You can easily expect that ProspectAccount will receive create_from_vendor with the correct arguments.

As for testing ProspectAccount.create_from_vendor(epoch, vendor) — now that you’ve reified this as a main interface, I’d update your existing tests for ProjectAccount#create so that they call ProspectAccount.create_from_vendor(epoch, vendor) rather than creating a ProspectAccount instance and calling create on it. In effect, this further strengthens create_from_vendor‘s role as a (or perhaps the) main public API of ProspectAccount and treats create as a private implementation detail even though it’s not actually a private method according to Ruby’s visibility.

Regarding any_instance: I’d argue that expect_any_instance_of(ProjectAccount).to receive(:create).with(epoch) is worse than the more explicit form of:

allow(ProjectAccount).to receive(:new).with(vendor).and_return(account = instance_double(ProjectAccount))
expect(account).to receive(:create).with(epoch)

IMO, this is better for a couple reasons:

• It’s more honest about the complexities of the communication patterns. One of the goals of mock objects is to put design pressure on you towards better interfaces and simpler communication patterns. any_instance is a convenience that makes a complex communication pattern (creating an instance, and then calling a method on that instance) look simple in the test when it’s really not. Usage of mocks to encode complex interactions in your tests is usually a net loss in the long term. It’s better, IMO, for your test to reflect the reality of the complex communication pattern…or even better, use that design feedback to come up with a simpler communication pattern (such as the create_from_vendor interface we’ve discussed!).
• The original any_instance expectation merely specifies that create is called with the correct epoch value. It doesn’t care that the instance that received the message is one with the expected vendor — but isn’t the vendor just important? Wouldn’t it be a bug if the implementation created a ProjectAccount with a different vendor than the desired one and called create(epoch) on that? The any_instance solution is unable to detect this kind of bug, but the longer, more verbose version I put above is.

All that said: I’d still consider this a bit smelly — as Aaron mentioned, stubbing a method on the object-under-test is smelly. I don’t really think it’s necessary given the solution I proposed above.

Does that help?

HTH,
Myron

​

--
You received this message because you are subscribed to the Google Groups "rspec" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rspec+un...@googlegroups.com.
To post to this group, send email to rs...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/rspec/87099f16-88f8-423f-a009-41f5598bb7a7%40googlegroups.com.

[Marlin Pierce]

Yes that does help.

I understand the second bullet point.  It might not be a bug if the create was called with a different vendor object, but it would be unless the other vendor object represented the same vendor in reality, (same name, same primary key).

I'm not sure I grok how the communication pattern is complex.

Maybe create is not the right name for the method.  It's more like write, or transmit.  Really, its transmit the call to the server for a create action.

The design of the code is for a ProspectAccount object to initialize its state from a given Vendor object.  Then, knowing its state, send the create information to the web server acting as our queuing system.

Going to lengths to avoid any_instance and its variations, I had come up with:

let(:vendor) { ::Vendor.new(...) }
let(:prospect) { ProspectAccount.new(vendor) }

it 'calls create instance method' do
  allow(ProspectAccount).to receive(:new).and_return(prospect)
  expect(prospect).to receive(:create).and_return(true)
 
  ProspectAccount.create_from_vendor(epoch, vendor)
end

I bulk a little at stubbing the ::new method, as it is a fundamental to the OOP language.

One strange point, (and maybe I'm not grokking what you are saying) is that it seems that your example of returning a double, is stubbing the object-under-test, and then you stub the :create method, which is stubbing the method on the object-under-test.

Further, I can see problems with stubbing the object-under-test, but not with writing an expectation to call a method when I am trying to verify that the method gets called.

class BatchController

  def create_accounts(epoch, start_time)
    vendors = Vendor.new_vendors(start_time)

    vendors.each do |vendor|
      ProspectAccount.new(vendor).create(epoch)
    end
  end

...
end

describe '#create' do

  let(:vendors) { [ ::Vendor.new(...), ::Vendor.new(...), ::Vendor.new(...) ] }
  let(:prospect) { ProspectAccount.new(vendor) }

  it 'calls create instance method' do
    allow(Vendor).to receive(:new_vendors).and_return(vendors)
    expect_any_instance_of(ProspectAccount).to receive(:create).exactly(3).times.and_return(true)

    BatchController.create(epoch, start_time)
  end
end

Here, it does not seem that I'm stubbing the object-under-test.

Maybe I am, since I am stubbing every instance of ProspectAccount, and I'm testing if those objects call the #create method.

[Aaron Kromer]

Thank you for all of the extra details.

BTW, The create is not the ActiveRecord create. Also we are not using Rails, just ruby. We don’t have a view. The closest we come to a controller object/class is the class that the create_accounts method is in. Say for the sake of argument, that that class is named BatchController.

That’s fine. We don’t need to pretend it is Rails at all. I was just going off the very common AR create interface on that guess.

To avoid using any_instance, I rewrote the call inside the loop to ProspectAccount.create_from_vendor(epoch, vendor), but that only moves the problem to the create_from_vendor method.

It’s difficult to judge things based only off of this small code snippet. Without a better understanding of your domain models (nothing to do with Rails or ActiveRecord) and how they are related I can only guess at some alternatives. Given your latest sample it seems really that you are writing some sort of service object.

I agree with Myron. Given it seems that a prospect account is tightly coupled to a vendor and an epoch; and you’ve stated create is of your own API design, I would pose the question: _”Is there a need to ever call create directly on a ProspectAccount instance; outside of create_from_vendor?”

If the answer is “no”. I would strongly suggest going with what Myron proposed. Make create a private implementation detail and promote the tests for it up to the create_from_vendor call. Then only use create_from_vendor in the code.

For example:

class ProspectAccount

  def self.create_from_vendor(epoch, vendor)
    new(vendor).create(epoch)
  end

private

  def create(epoch)
    # ...
  end
end

RSpec.describe ProspectAccount do
  describe "creating an account from a vendor with epoch" do
    # move all tests you had for `create` here
  end
end

Given all of your well thought out reasoning behind the design decisions so far, if the answer is “yes” we could approach things from another angle. Based only on these snippets, it seems you cannot have a ProspectAccount instance without a Vendor instance. We could also consider an API design with:

class Vendor
  def self.generate_prospects(start_time)
    new_vendors(start_time).map { |v| ProspectAccount.new(v) }
  end
end

RSpec.describe Vendor do
  it "with no matching start time generating prospects returns an empty array" do
    expect(Vendor.generate_prospects(some_non_matching_time)).to eq []
  end

  it "with a matching start time generating prospects returns the new prospects" do
    # This may need a custom matcher or some sort of equality checker for prospects
    expect(Vendor.generate_prospects(matching_time)).to match_array some_expected_prospects
  end
end

That wraps the factory up to the thing which the prospect depends on. This frees up your service object to be:

class BatchController
  def self.create_accounts(epoch, start_time)
    Vendor.generate_prospects(start_time).each do |prospect|
      prospect.create(epoch)
    end
  end

end

RSpec.describe BatchController do
  context "with some prospects" do
    it "creating accounts transmits each prospect" do
      a_start_time = :start_time
      an_epoch = :epoch
      prospects = [spy(ProspectAccount), spy(ProspectAccount)]
      allow(Vendor).to receive(:generate_prospects).with(a_start_time).and_return(prospects)

      BatchController.create_accounts(an_epoch, a_start_time)

      prospects.each { |prospect| expect(prospect).to have_received(:create).with(an_epoch) }
    end
  end
end

​

--
You received this message because you are subscribed to the Google Groups "rspec" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rspec+un...@googlegroups.com.
To post to this group, send email to rs...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/rspec/bcb391cc-989b-499a-83b5-6fb1174220e7%40googlegroups.com.

[Myron Marston]

I’m not sure I grok how the communication pattern is complex.

It’s not super complex but calling 2 methods (new followed by create on the returned object) is more complex than calling one (create_from_vendor). any_instance makes this communication pattern look simpler than it actually is in the test, and is thus less honest than the more verbose form.

Going to lengths to avoid any_instance and its variations, I had come up with:

Is there a reason you feel the need to use mocking or stubbing at all when testing create_from_vendor? At some point, you’ve got test the underlying operations w/o mocking or stubbing them…so presumably you have tests for the create method. Why not simply update that test to use ProjectAccount.create_from_vendor rather than ProjectAccount#create? Then there’s no need for the gymnastics of any_instance or the stubbing of new, and your test makes it explicit that clients that need this operation should use create_from_vendor to achieve it.

I bulk a little at stubbing the ::new method, as it is a fundamental to the OOP language.

FWIW, I don’t consider new to be special in any way. It’s not a keyword. It’s just a message like any other.

One strange point, (and maybe I’m not grokking what you are saying) is that it seems that your example of returning a double, is stubbing the object-under-test, and then you stub the :create method, which is stubbing the method on the object-under-test.

To be clear, the example snippet that I sent (e.g. stubbingnew and the expecting create) is not what I recommend. I recommend you have your tests for ProjectAccount drive the interaction with it via create_from_vendor with out using mocking or stubbing (or perhaps injecting some test double collaborators as needed…but certainly not stubbing or mocking any ProjectAccount class or instance methods). And then in any other tests of code that needs to create a project account from a vendor, you can simply and easily mock ProjectAccount.create_from_vendor.

The snippet was just meant to show that if it’s important to you to not update your interfaces to create easily mockable “seams” between your components, there’s an alternative to any_instance.

Further, I can see problems with stubbing the object-under-test, but not with writing an expectation to call a method when I am trying to verify that the method gets called.

To be clear, code smells are indication of possible problems, and not a judgement that every single example of that smell is a problem. Anytime you stub or use a message expectation on the object-under-test, you are modifying it to behave differently than the real object behaves, and tests of such objects are generally less trustworthy than tests of unmodified objects. More generally, I fine my tests easier to reason about when there’s an explicit separation between the object I am testing (which has not been modified with a test-specific extension) and the surrounding environment (which generally has been).

All that said: a lot of this depends upon your testing philosophy and what level of granularity you are going for. I aim for unit tests that are very fast (largely by avoiding IO dependencies or using injected test doubles to replace them) while still being kinda course-grained so that the test is coupled to a very small API surface area to facilitate future refactoring. Others view the “unit” of a “unit test” as being an individual method, and in such a philosophy, you might not view stubbing the object-under-test as problematic since you are focused on testing a particular method, not a particular object.

Myron

​