how to test that methods are called inside a block?

[Joe Van Dyk]

Say I have the following method:

      def run

        transaction do

          mark_non_eligible

          make_invoice_batch

          send_batch_email

        end

      end

How can I test that with rspec's mocks?

Joe

[Myron Marston]

There's a rarely-used feature of `and_yield` that can help you with this.  If you pass a block to `and_yield`, RSpec will pass your block an object that it will use to instance_eval the `transaction` block, allowing you to set message expectations on it.  I put together an example gist:

https://gist.github.com/myronmarston/e79cbff12ce51b54814b

This works, but bear in mind there are a number of code smells here (mocking the object-under test, making a test that simply mirrors the implementation, etc).  IMO, you'd be better off testing this with an integrated test that hits the DB.  You could, for example, stub whatever collaborator `send_batch_email` delegates to so that it fails, and check that the database updates performed by `mark_non_eligible` and `make_invoice_batch` are rolled back.

HTH,

Myron

[Nathan Wenneker]

I know I'm replying to an old thread, but it seemed relevant.

Suppose we want to unit test in pure isolation a method that passes a block to a collaborator.  For example:

    def baz
      CollaboratorA.foo do
        CollaboratorB.bar
      end
    end

It's important not only that `baz` calls `CollaboratorB.bar`, but that it does it inside the block. Myron's gist from a year ago answers this question well.  However, what if we take this a step further, and say that we want to call `CollaboratorB.bar` once inside the block and once outside the block like this:

    def baz
      CollaboratorA.foo do
        CollaboratorB.bar
      end
      CollaboratorB.bar
    end

Furthermore, let's suppose we want to assemble the return values from these two `bar` calls.  This isn't a Rails specific question, but I'll use `ActiveRecord::Base.unscoped` (assuming a default_scope is set) as a real-world example that I hope will be easily understood:

    def fetch_products
      visible_products = Product.all
      all_products = Product.unscoped do
        Product.all
      end
      { visible_products: visible_products, all_products: all_products }
    end

I know that an isolated test for this will be tightly coupled to the implementation, but assuming that's what we want to do, I imagined a new a new `inside` and `outside` API for rspec-mocks, and this is how I would test `fetch_products`:

    it "fetches products" do
      all_products = double("all products")
      visible_products = double("visible products")

      unscoped = allow(Product).to receive(:unscoped).and_yield
      allow(Product).to receive(:all).inside(unscoped).and_return(all_products)
      allow(Product).to receive(:all).outside(unscoped).and_return(visible_products)

      expect(fetch_products).to eq({
        visible_products: visible_products,
        all_products: all_products
      })

    end

If you didn't have return values to compare against, you could also write something like:

    expect(Product).to have_received(:all).inside(unscoped).once
    expect(Product).to have_received(:all).outside(unscoped).once


So I have a two questions:
 
 1. Is there a straightforward way to write this spec using the current API?
 2. If not, would you be open to a PR that adds the `inside` and `outside` API, or open to discussing an alternative API that would make this kind of spec feasible?

I have a working prototype locally that enhances `@messages_received` to track what existing message expectations are running an implementation when a subsequent message is called, which can then be compared to to constraints.

Thank you,

Nathan

[Myron Marston]

> However, what if we take this a step further, and say that we want to call `CollaboratorB.bar` once inside the block and once outside the block like this:

Remember that rspec-mocks allows you to pass an implementation block, which flexibly allows you to do pretty much anything.  In this case you could do:

``` ruby

sequence = []

allow(CollaboratorB).to receive(:bar) { sequence << :bar }

allow(CollaboratorA).to receive(:foo) do |&block|

  sequence << :foo_start

  block.call

  sequence << :foo_end

end

baz

expect(sequence).to eq [:foo_start, :bar, :foo_end, :bar]

```

For the `fetch_product` case, you could do something similar.  Although, personally I wouldn't test a method like that with mocks at all: I'd tend to integration test it.

Anyhow, as for adding something more explicit to support this directly from the rspec-mocks API: I think we'd want to hear that this is a more common problem with our users to warrant adding that complexity to RSpec.   I can't think of a time I've ever wanted that and I can't think of any user requesting it before.  It's a complex thing and the `inside`/`outside` thing you've come up with reads pretty well but it's pretty different from the rest of the rspec-mocks API to get a result from setting up a prior stub and use it in a later one.

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/c9704cee-9a7c-404f-837f-514d90c89dcc%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Nathan Wenneker]

Hi Myron,

Thanks for your reply -- yes that's helpful!

Essentially your local variable `sequence` is a poor man's `@messages_received`.   I hadn't thought of tracking that locally within the spec itself, but that definitely is the key to solving my problem without enhancing the existing API, albeit not as succinctly.  For anyone who stumbles on this in the future, I've included my new implementation of testing `fetch_products` at the very bottom of this post.

As for enhancing the API, one of your concerns is that the need to test blocks like this is too rare.

That's definitely your call, but I'll at least explain my line of thinking for you to consider, if you wish. Sorry, this is a long post...

I presume you'll agree that code within blocks itself is not rare, so one way or the other we need to test it   This really becomes a question of whether it's better to test them in isolation or better to test them in integration.

I want to compare this with a concrete example:

Suppose I have several commands that need to occur in a transaction:

def handle_real_estate_deal(buyer, seller, realtor1, realtor2, amount)
   Transaction.run do
     commission = amount * 0.06
     buyer.withdraw(amount)
     seller.deposit(amount - commission)
     realtor1.deposit(commission / 2)
     realtor2.deposit(commission / 2)
   end
 end

Now forgetting the transaction for a moment, an isolated test might look like this:

it "calls collaborators with correct amounts" do
   expect(buyer).to receive(:withdraw).with(100_000)
   expect(seller).to receive(:deposit).with(94_000)
   expect(realtor1).to receive(:deposit).with(3_000)
   expect(realtor2).to receive(:despoit).with(3_000)
   handle_real_estate_deal(buyer, seller, realtor1, realtor2, 100_000)
 end

However, there is a lot of subtle (but important) behavior that this method gets "for free" by running inside `Transaction.run`.

1. if the first command fails, nothing is committed
2. if the second command fails, nothing is committed
3. if the third command fails, nothing is committed
4. if the fourth command fails, nothing is committed

So I could write a series of small integration specs (integrating with `Transaction.run`, but mocking the other collaborators) to ensure that nothing is committed when various failures occur

Here's the first:

it "if the buyer withdraw fails, nothing is committed" do
   allow(buyer).to receive(:withdraw).and_raise
   allow(seller).to receive(:deposit)
   allow(realtor1).to receive(:deposit)
   allow(realtor2).to receive(:desposit)
 
   handle_real_estate_deal(buyer, seller, realtor1, realtor2, amount)
 
   expect(Transaction.commits).to be_empty
 end

Here's the second:

it "if the seller deposit fails, nothing is committed" do
   allow(buyer).to receive(:withdraw)
   allow(seller).to receive(:deposit).and_raise
   allow(realtor1).to receive(:deposit)
   allow(realtor2).to receive(:desposit)
 
   handle_real_estate_deal(buyer, seller, realtor1, realtor2, amount)
 
   expect(Transaction.commits).to be_empty
 end

The other specs would be similar, so I won't actually write them out here.  Of course you might be able to use metaprogramming to make this a little easier


These integrated tests are really testing behavior that comes from `Transaction.run`, which presumably is well-tested somewhere else.

If instead I write an isolated test that verifies the commands are being called from within a `Transaction.run` block, I get essentially the same coverage by modifying my original isolated test to look like this:

it "calls collaborators with correct amounts from within a transaction block" do
  transaction = allow(Transaction).to receive(:run).and_yield

  expect(buyer).to receive(:withdraw).with(100_000).inside(transaction)
  expect(seller).to receive(:deposit).with(94_000).inside(transaction)
  expect(realtor1).to receive(:deposit).with(3_000).inside(transaction)
  expect(realtor2).to receive(:deposit).with(3_000).inside(transaction)
  handle_real_estate_deal(buyer, seller, realtor1, realtor2, 100_000)
end

As long as `Transaction.run` does what it's supposed to do, and as long as my boundaries are good, a few modifications to an existing test provides the same coverage as adding 4 integrated tests.  This is exacerbated further if `Transaction.run` has additional behavior beyond just preventing commits.

So I suppose I just don't fully understand why integrated tests are more ideal than isolated tests for something like this.  Maybe if we made it easier to test this way, it wouldn't be so rare?  For highly-critical pieces of your application, perhaps you want a full-set of integrated tests anyway, but for less-critical pieces, being able to get fairly thorough coverage without much boilerplate is a big win, isn't it?


As for passing one stub into another: I agree it's odd to pass one stub into another. I had considered just using the method name (i.e. `:unscoped`) itself as the identifier (e.g. `allow(Product).to receive(:all).inside(:unscoped)`), but it's possible to have more than one stub for `:unscoped`, possibly with different arguments, so I needed a way to uniquely identify that I was inside the correct block. Alternatively the user could name the block himself (i.e. something like `allow(Product).to receive(:unscoped).and_yield_as(:some_user_chosen_block_name)`, but that doesn't necessarily seem preferable.


Finally, for sake of anyone stumbling on this later, here is one way to stub a method to return different values depending on whether it is called from within a given block or not, piggybacking on Myron's suggestion to use a local variable in the spec itself to track messages (i.e. like he did with `sequence`)

def fetch_products
  visible_products = Product.all
  all_products = Product.unscoped do
    Product.all
  end
  { visible_products: visible_products, all_products: all_products }
end

it "fetches products" do

   all_products = double("all products")
   visible_products = double("visible products")

 
   block_stack = []
 
   allow(Product).to receive(:unscoped) do |&block|
     block_stack.push(:unscoped)
     result = block.call
     block_stack.pop
     result
   end
 
   allow(Product).to receive(:all) do
     if block_stack.include?(:unscoped)
       all_products
     else
       visible_products
     end
   end

 
   expect(fetch_products).to eq({
     visible_products: visible_products,
     all_products: all_products
   })
 
 end

Thank you for your help Myron!,


Nathan