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 On Thu, May 28, 2015 at 10:24 AM, Marlin Pierce <[email protected]> wrote: > 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. > > > > >> -- > 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 [email protected]. > To post to this group, send email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/rspec/87099f16-88f8-423f-a009-41f5598bb7a7%40googlegroups.com > <https://groups.google.com/d/msgid/rspec/87099f16-88f8-423f-a009-41f5598bb7a7%40googlegroups.com?utm_medium=email&utm_source=footer> > . > > For more options, visit https://groups.google.com/d/optout. > -- 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 [email protected]. To post to this group, send email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/rspec/CADUxQmsUDTrVY1KBAAEngjTmncHb5VHgAxM3YZmuSrRZ-N6GkQ%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
