return what the block returned when using yield

[eatwithforks]

I have a method that yields to a block and I want to stub out that method but still what the block returned without using an explicit return.

def foo
  Dir.chdir { "hello" }
end

Dir.stubs(:chdir) yields
foo.must_equal "hello"

[eatwithforks]

/cc @grosser

[floehopper]

@eatwithforks

Thanks for opening this issue and for your interest in Mocha. I may have misunderstood, but I think your issue is very similar to the one described in #227 or possibly more relevantly in this older issue. Do you think the latter is describing the same issue as yours?

If yes, I think my explanation in this comment is probably the most relevant one. Does that explanation make sense?

If no, can you explain a bit more what makes your issue different from that one? Thanks.

[grosser]

ah yes, #227 is what we had in mind
I also like the logic of returning result unless a return value was given ... but for backwards-compatibility a new yields_with_return could also work 🤷

[collinsauve]

👋 Hello! Just a bump on this issue. I've experienced the same problem numerous times using Mocha. We're currently required to explicitly set the .returns on an expectation, and this does not allow us to test that the block actually returned the expected value.

As mentioned above, #227 looks like it solves this problem. It was previously closed as a concrete example was not provided. Does this issue have an appropriate example? If not I can provide another.

What's the best way to go about getting the change proposed on that PR merged to Mocha? I'd be willing to open a new PR myself if that would be best.

[floehopper]

@collinsauve

Does this issue have an appropriate example?

The example given in the description of this issue didn't really help much - it doesn't seem like a very realistic example. In particular, I think I want to understand what behaviour we want to be able to test with this new feature.

If not I can provide another.

It would be helpful if you have any realistic examples which could help me understand what you're trying to do.

[collinsauve]

I've put together an example that is much closer to where I have (most recently) encountered this issue:
https://github.com/collinsauve/mocha-issue-493-example

You can run ./example.rb to see the system-under-test operating properly, and ./example-test.rb to execute the test.

Test currently passes only because I forced the yield to return something that the test defines. A proper test for this should not ignore the return value of the block. Now that block's behavior is not properly tested.

Relevant snippets from the repo included here:

  def self.get_txt(domain)    
    resources =
      Resolv::DNS.open do |dns|
        dns.getresources(domain, Resolv::DNS::Resource::IN::TXT)
      end

    resources.map { |r| r.data }
  end

def test_get_txt
    records = TXT_RECORDS.map do |r|
      record = mock
      record.expects(:data).with.returns(r)
      record
    end

    dns = mock    
    dns.expects(:getresources).with(DOMAIN, Resolv::DNS::Resource::IN::TXT).returns(records)

    Resolv::DNS
      .expects(:open)
      .yields(dns)
      .returns(records) # I should not have to do this, and it allows bad implementations to not actually return the results properly

    result = Example.get_txt(DOMAIN)

    assert_equal TXT_RECORDS, result
  end

[collinsauve]

If you'd like I can also modify the system-under-test to be misbehaving and yet the test still passes due to this bug. I'm thinking by now this point has been demonstrated, but LMK.

[floehopper]

@collinsauve

Thanks. That's really helpful. I think there are a few different things going on here.

In your example code it looks like you're regarding the implementation of Example.get_txt and the implementation of the block passed to Resolv::DNS.open as a single concern that you want to test in combination in a single test. While this makes a lot of sense to me in this particular scenario, in scenarios where the block implementation is more complex, I often extract the instantiation of the Proc into its own method so I can unit test it separately. However, I can see that where you want a single test like this, the Mocha functionality that you have requested would be useful.

It's also important to recognise that not every Ruby method implementation yielding to a block, returns the block's return value. So, although I acknowledge it is a relatively common pattern, Mocha needs to be able to stub method implementations where this is not the case and it makes more sense to explicitly define the return value in Expectation#returns.

If I simply change Expectation#yields to return the block's return value as in #227, I think there will be backward compatibility problems, so I'm leaning towards adding a new Expectation#yields_with_return method to implement the new behaviour as suggested by @grosser in an earlier comment. Would this solve your problem?

[floehopper]

I've been thinking about this a bit more and spiking a bit on possible solutions. In doing so I noticed that (unsurpisingly) the code in #227 is very out-of-date and would need re-writing.

Anyway, I'm wondering whether it would be clearer to always capture block return values and additionally implement something like Expectation#returns_block_return_value. This makes more sense to me, because it's the behaviour of the return value from the stubbed method that we want to change.

However, this has made me realise that I might be opening a bit of a can of worms because someone might legitimately wonder why they can't implement more sophisticated behaviour, e.g. return value_returned_from_block * 2 + 1.

Also #227 suggests that a call to Expectation#multiple_yields should mean that the return value is the return value from the last block. While I can see this is a common scenario, I can see why someone might want to do something different.

And what should happen when a stubbed method yields to a block and the method is invoked multiple times yielding different values to the block each time? At the moment you can do something like this:

foo.stubs(:bar).yields(1).returns(1).then.yields(2).returns(4)

assert_equal 1, foo.bar { |x| x * x }
assert_equal 4, foo.bar { |x| x * x }

Presumably we'd want to be able to do something like this:

foo.stubs(:bar).yields(1).returns_block_return_value.then.yields(2).returns_block_return_value

assert_equal 1, foo.bar { |x| x * x }
assert_equal 4, foo.bar { |x| x * x }

Anyway, I think this is going to need some more thought.

[collinsauve]

Ah, yes. I am conflating two things:

• Being able to assert that the yielded block returns the expected value
• Defining what the stubbed method returns

Occasionally these are the same, but not always, and we should allow support for both of those things.

Attacking this problem by defining the returns to be the result of the block is incorrect. Rather, we need a way to assert the return value of the block independently.

[collinsauve]

Just brainstorming here but is there some interface on yields that we could add to define the expected result of the yield?

expectation = foo.stubs(:bar)
expectation.yields.with(1)
expectation.returns(1)

Perhaps with is an overloaded term as it currently means "expected arguments" on an expectation, but hopefully this demonstrates the idea enough to brainstorm a better name for it.

[floehopper]

@collinsauve Thanks for your replies.

Ah, yes. I am conflating two things:

• Being able to assert that the yielded block returns the expected value
• Defining what the stubbed method returns

I agree that these two things are being conflated.

Occasionally these are the same, but not always, and we should allow support for both of those things.

I agree that the two things are not always the same and I'm coming round to the idea that it would be nice to add support for the first one.

Attacking this problem by defining the returns to be the result of the block is incorrect. Rather, we need a way to assert the return value of the block independently.

I don't think I'd go so far as saying that using something like the Expectation#returns_block_return_value is incorrect. However, it's useful to hear that you don't find it as logical/unsurprising as I do.

I find it interesting that you've said you want to assert the value. Is that definitely the only thing that you want to be able to do? My impression from you (and others who have commented on this issue) is that you also want the stubbed method to actually return the block return value. Here's some code to explain what I mean:

foo.stubs(:bar).yields.asserts_block_returns(1).returns(2)

foo.bar { 1 } # => no error; stubbed method `bar` returns the value `2`
foo.bar { 3 } # => unexpected invocation error; test fails before any value is returned from stubbed method `bar`

Whether or not this is actually practical to implement and whether or not the naming is sensible, it believe it satisfies your desire to assert the block return value. However, I suspect it may not be sufficient to address all the concerns raised in this issue, because I think people want a way for a stubbed method to actually return the block return value whether or not it satisfies some assertion. Or am I missing something?

Just brainstorming here but is there some interface on yields that we could add to define the expected result of the yield?

I'm not quite sure what you're getting at here. Expectation#yields is one of the methods that makes up Mocha's fluent interface, i.e. it returns the Expectation instance to facilitate method chaining, and I wouldn't want to change that. So any new method will also need to be an instance method on Expectation that returns the Expectation instance, i.e. I don't want to introduce a variation of Expectation#yields that returns a different object with a different interface.

expectation = foo.stubs(:bar)
expectation.yields.with(1)
expectation.returns(1)

Perhaps with is an overloaded term as it currently means "expected arguments" on an expectation, but hopefully this demonstrates the idea enough to brainstorm a better name for it.

You're right that I wouldn't want to overload Expectation#with for this very different use case. If we definitely want to go in this direction, I think it would make sense to go with something more like Expectation#yields_with_return as @grosser suggested in an earlier comment, although it's not obvious that implies any kind of assertion.

In any case, I'm a bit unsure about adding an assertion, because there isn't really any precedent for this in the Expectation API and so it seems a bit surprising to me. I know it's not a perfect comparison, but Expectation#with doesn't directly assert that the arguments passed to a stubbed method match; it simply determines whether that expectation matches the current invocation. This is because you can define multiple expectations for the same stubbed method.

Anyway, this is all very useful food for thought.