.yields() failing when the caller does not provide a block breaks legitimate tests

[yemartin]

.yields() failing when the caller does not provide a block breaks legitimate tests

I have a library with an expensive method (API calls) that both yields and returns. In some tests, both the yielding and the returning aspects are used, so I need to stub both. This used to work fine in 1.9.0, but broke when upgrading to 1.10.1. I traced it down to this in the 1.10.0.alpha release notes (emphasis mine):

Make Expectation#yields & Expectation#multiple_yields fail when the caller of the stubbed method does not provide a block. This is a change to an undocumented aspect of the public API's behaviour. If this causes your tests to fail, then fix it by removing the unnecessary call to Expectation#yields or Expectation#multiple_yields - thanks to @nitishr (#382)

But my calls to .yields are legitimate and necessary, I cannot just remove them. So I am left with broken tests, and I am not sure how to fix them. Maybe the change in #382 needs to be reevaluated?

How to reproduce

Here is a code sample that illustrates how one may need both .returns and .yields in the same test:

# my_service.rb

class MyService
  def self.expensive_method
    # Some very expensive calls here
    yield "yielded" if block_given?
    return "returned"
  end

  def self.call
    results = []
    expensive_method do |yielded|
      results << yielded
    end
    results << expensive_method
  end
end

# test_my_service.rb

class TestMyService < Minitest::Test

  def test_returns
    MyService.stubs(:expensive_method).returns("returned")
    assert_equal "returned", MyService.expensive_method
  end

  def test_yields
    MyService.stubs(:expensive_method).yields("yielded")
    MyService.expensive_method do |yielded|
      assert_equal "yielded", yielded
    end
  end


  def test_both_returns_and_yields
    MyService
      .stubs(:expensive_method)
      .returns("returned")
      .yields("yielded")
    call_result = MyService.call
    assert_includes call_result, "returned"
    assert_includes call_result, "yielded"
  end
end

With mocha 1.9.0:

# Running:

...

Finished in 0.001714s, 1750.2917 runs/s, 3500.5834 assertions/s.

3 runs, 6 assertions, 0 failures, 0 errors, 0 skips

With mocha 1.10.1:

# Running:

E..

Finished in 0.001527s, 1964.6365 runs/s, 1309.7577 assertions/s.

  1) Error:
TestMyService#test_both_returns_and_yields:
LocalJumpError: no block given (yield)
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/expectation.rb:571:in `block in invoke'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/invocation.rb:22:in `block in call'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/single_yield.rb:10:in `each'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/invocation.rb:20:in `call'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/expectation.rb:571:in `invoke'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/mock.rb:315:in `method_missing'
    /Users/yemartin/.gem/ruby/2.3.1/gems/mocha-1.10.1/lib/mocha/stubbed_method.rb:61:in `block in define_new_method'
    /Users/yemartin/Desktop/mocha_yields_and_returns/my_service.rb:13:in `call'
    ./test_my_service.rb:27:in `test_both_returns_and_yields'

3 runs, 2 assertions, 0 failures, 1 errors, 0 skips

[nitishr]

Thanks for reporting, @yemartin.

If you're able to make a few changes to either the test or the application code, I think you work with the new behavior which I still think is more correct/better than the old behavior. Here'a a few observations/suggestions I have:

The error you're getting is due to the last line in this method not giving a block to expensive_method:

  def self.call
    results = []
    expensive_method do |yielded|
      results << yielded
    end
    results << expensive_method
  end

Without much context, it seems to me you're calling expensive_method one time too many, once for the yielded value and once for the return value, only to accumulate both of them. But you could do that with a single call like so:

  def self.call
    results = []
    results << expensive_method do |yielded|
      results << yielded
    end
  end

This would stop your test failing as well, because expensive_method isn't called without a block anymore.

Alternatively, you could change your test from:

  def test_both_returns_and_yields
    MyService
      .stubs(:expensive_method)
      .returns("returned")
      .yields("yielded")
    call_result = MyService.call
    assert_includes call_result, "returned"
    assert_includes call_result, "yielded"
  end

to:

  def test_both_returns_and_yields
    MyService.stubs(:expensive_method).returns("returned")
    MyService.stubs(:expensive_method).yields("yielded").returns("returned")
    call_result = MyService.call
    assert_includes call_result, "returned"
    assert_includes call_result, "yielded"
  end

This sets up MyService.expensive_method to yield and return on the first invocation, and to only return on the second invocation. See the documentation for Mocha::Mock for further details (FYI, the invocation order is likely to be reversed in a future version).

I've tried both of those approaches and they get the tests passing. Does any of those approaches work for you?

[yemartin]

Hi @nitishr, and thank you for the ideas.

I won't pretend to understand why you think the new behavior is more correct, this goes way above my head... But my use case is this: multiple calls, in a potentially unpredictable sequence, to a method that both yields and returns, and that needs stubbing.

1. On changing application code:

Changing application code is unfortunately not an option, and anyway, it is very bad practice to modify production code in order to satisfy the idiosyncrasies of testing. I am, however, totally open to changing the test code.

2. But changing the test code is not practical either:

The problem with the test code change is that it requires knowing the exact number, type (with or without block), and order of all invocations. That is easy to do in this simplistic example, but my actual broken test is a high level integration test. The method is called many times during the test, so hardcoding that exact sequence of invocations as stubs would be quite tedious. Worse, it would also be a terrible practice, as it means totally coupling a feature test to the exact implementation, making the test extremely brittle since any refactoring would likely break it. And I am not even sure it is technically possible to hardcode the sequence of invocations: test randomness may cause the exact sequence to change from one test run to the next.

I am open to other ideas, but so far, by the looks of things, Mocha 1.10 has broken the use case I describe.

[floehopper]

@yemartin

Thank you for reporting the issue and for giving such clear examples. I'm sorry this has caused you problems. In hindsight, although the old behaviour was undocumented, I probably should have added a deprecation warning before changing it - sorry!

I think the new behaviour does makes sense, but I think we probably need to do a better job of explaining why.

I'm going to investigate how feasible it would be to reinstate the old behaviour somehow. In the meantime, I'm hoping you can pin your project to Mocha v1.9 to avoid the problem until we come up with a solution.

[yemartin]

Thank you for the extra info @floehopper. Even though I was trying to avoid it, we will be locking Mocha at 1.9 for now.

Indeed, more explanation would help as I still don't get why the new behavior makes more sense. The idiom if block_given? is pretty common in Ruby... Doesn't this call directly for a yields stubbing where the block is optional?

One important note: the mocking in this test is there to prevent expensive API calls, not to set expectations on the invocations. In RSpec terminology, I need allow(...).to receive (instead of expect(...).to receive).

Anyway, I wrote same tests using RSpec mocking and got a similar error. So RSpec seems to agree with you all, but I would not take that as the definitive answer: the very reason we chose Mocha over RSpec for mocking was because Mocha was simply more powerful at the time. So let's not consider RSpec the reference behavior and jump to conclusions. And indeed, until 1.9, Mocha was still more powerful as it allowed us to support the use case I describe in this issue.

For reference, here is the RSpec implementation:

# my_service_spec.rb

describe MyService do
  it "returns" do
    allow(MyService).to receive(:expensive_method).and_return("returned")
    expect(MyService.expensive_method).to eq "returned"
  end

  it "yields" do
    allow(MyService).to receive(:expensive_method).and_yield("yielded")
    MyService.expensive_method do |yielded|
      expect(yielded).to eq "yielded"
    end
  end

  it "both returns and yields" do
    allow(MyService)
      .to receive(:expensive_method)
      .and_yield("yielded")
      .and_return("returned")
    call_result = MyService.call
    expect(call_result).to include("returned")
    expect(call_result).to include("yielded")
  end
end

This gives me the following:

Randomized with seed 61032
..F

Failures:

  1) MyService both returns and yields
     Failure/Error: call_result = MyService.call
       #<MyService (class)> asked to yield |["yielded"]| but no block was passed
     # ./my_service.rb:13:in `call'
     # ./my_service_spec.rb:24:in `block (2 levels) in <top (required)>'

Finished in 0.01531 seconds (files took 0.13957 seconds to load)
3 examples, 1 failure

Failed examples:

rspec ./my_service_spec.rb:19 # MyService both returns and yields

[floehopper]

@yemartin

Thank you for the extra info @floehopper. Even though I was trying to avoid it, we will be locking Mocha at 1.9 for now.

👍

Indeed, more explanation would help as I still don't get why the new behavior makes more sense. The idiom if block_given? is pretty common in Ruby... Doesn't this call directly for a yields stubbing where the block is optional?

I will come back to you about this.

One important note: the mocking in this test is there to prevent expensive API calls, not to set expectations on the invocations. In RSpec terminology, I need allow(...).to receive (instead of expect(...).to receive).

Thanks for the clarification.

Anyway, I wrote same tests using RSpec mocking and got a similar error. So RSpec seems to agree with you all, but I would not take that as the definitive answer: the very reason we chose Mocha over RSpec for mocking was because Mocha was simply more powerful at the time. So let's not consider RSpec the reference behavior and jump to conclusions. And indeed, until 1.9, Mocha was still more powerful as it allowed us to support the use case I describe in this issue.

Thanks for trying this. That's useful information.

I've just opened #437 which provides a configuration option to revert to the v1.9 behaviour, albeit with a deprecation warning. If there's any way you could try out the reinstate-expectation-yields-behaviour branch locally in your tests, that would be great. I realise this is only a short-term fix, but I think it would leave us in a better place and if released would allow you to upgrade Mocha.

[floehopper]

Indeed, more explanation would help as I still don't get why the new behavior makes more sense. The idiom if block_given? is pretty common in Ruby... Doesn't this call directly for a yields stubbing where the block is optional?

I'm a bit sick at the moment, so I apologise if I haven't got my head round this properly. 🤒

I think the new behaviour is inline with the rest of Mocha in the sense that stubbing a method is intended to provide a deterministic/explicitly-defined alternative implementation for the stubbed method. The old behaviour meant that the stubbed method would yield in one case, but not in the other and could thus be regarded as not so deterministic/explicitly-defined.

The new behaviour might be useful to some people, because if they're getting a LocalJumpError it might be an indication the stubbed method is not being invoked with a block when it should be or because the stubbed method should not be yielding (i.e. the test setup is unrealistic).

However, given your comments, I'm wondering whether what we're actually missing is some way to match invocations that do or don't have a block given, something along the following lines:

  def test_both_returns_and_yields
    MyService
      .stubs(:expensive_method)
      .with_block_given
      .yields("yielded")
      .returns("returned")
    MyService
      .stubs(:expensive_method)
      .with_no_block_given
      .returns("returned")
    call_result = MyService.call
    assert_includes call_result, "returned"
    assert_includes call_result, "yielded"
  end

I realise that's a lot more verbose, but I like the fact that it's more explicit. And if there are common cases we can always come up with shortcuts for those.

I haven't thought this through in a lot of detail and in particular I haven't thought about whether it would be possible to implement this in Mocha, but on first glance I think it should be.

Another alternative approach might be to provide a new Expectation#yields_if_block_given method in addition to the new-style Expectation#yields method. However, I think we'd need an equivalent for Expectation#multiple_yields which would be a bit ugly.

What do you think?

@nitishr Do you have any thoughts on this?

[yemartin]

The new behaviour might be useful to some people, because if they're getting a LocalJumpError it might be an indication the stubbed method is not being invoked with a block when it should be or because the stubbed method should not be yielding (i.e. the test setup is unrealistic).

Thanks a lot for the detailed explanation @floehopper ! This last part in particular was the "aha!" moment for me. I was just thinking in terms of stubbing, but from the expectation point of view, the new behavior does make sense.

I've just opened #437 which provides a configuration option to revert to the v1.9 behaviour, albeit with a deprecation warning. If there's any way you could try out the reinstate-expectation-yields-behaviour branch locally in your tests, that would be great. I realise this is only a short-term fix, but I think it would leave us in a better place and if released would allow you to upgrade Mocha.

Awesome, thanks for addressing this so quickly! I will try the reinstate-expectation-yields-behaviour branch today and will let you know how it goes.

However, given your comments, I'm wondering whether what we're actually missing is some way to match invocations that do or don't have a block given, something along the following lines:
[...]

Oh, I like these .with_block_given and .with_no_block_given! 👍 And I don't mind the verbosity at all: I totally agree that explicit is better than terse. I also agree that .yields_if_block_given sounds like a lesser approach: if the philosophy of Mocha is that stubbing a method is intended to provide a deterministic/explicitly-defined alternative implementation for the stubbed method, then these .with_block_given and .with_no_block_given fit very well, but .yields_if_block_given, not so much.

Thanks again @floehopper, and get well soon!

[floehopper]

@yemartin Thanks. I will try to get these changes (or something very similar) into a patch release as soon as I can.

[nitishr]

I have 2 contradicting lines of reasoning about this, and I'm still trying to make up my mind about which one leads to a more logical and coherent (and ideally simpler) outcome.

On the one hand, in response to

[...] my actual broken test is a high level integration test. The method is called many times during the test, so hardcoding that exact sequence of invocations as stubs would be quite tedious.

I'm not sure mocha is the the right tool for high level integration tests with an unknown/unpredictable/uncontrollable sequence of method calls. After all, we don't allow conditional return values through the returns specification - instead requiring an exact sequence of return values if you did want the returned value to be different for different calls. Not sure that yielded values - or yielding - should be conditional by that same logic. At a first glance at least, this would seem to take us dangerously close to mocha being used for faking, which it's explicity (at this point) not meant for.

On the other hand, I recognize we do support parameter matchers to specify different responses to different parameters. Perhaps you could consider blocks (or the presence or absence of them) to be a kind of parameter matching, thereby matching different expectations in each case. I guess @floehopper's suggestion/proposal follows that reasoning.

I'd find it helpful, @yemartin, to understand your situation/use case in a bit more detail in order to solidify our (or at least my) theory of mocha as a tool and it's usage (actual, recommended, supported, discouraged, prohibited, etc.).

Thoughts, @yemartin or @floehopper?

[yemartin]

Thank you for the input @nitishr. I will try to give a bit more details about what we are actually doing but it will take me some time to summarize without going into unnecessary details. So first, let me clear two important point:

1. Our return / yielded values are the same on every call, so the use case should fit Mocha

After all, we don't allow conditional return values through the returns specification - instead requiring an exact sequence of return values if you did want the returned value to be different for different calls.

I made the yielded and returned values different in the code sample for clarity, but in our actual test, we return / yield the same value, on every call. It is only the sequence of invocations that is intricate, and maybe even unpredictable. And looking at the Mock#stubs documentation (emphasis mine):

Adds an expectation that the specified method may be called any number of times with any parameters.

This allows for an unpredictable sequence of invocations, so I believe our use case fits Mocha well.

2. I am not sure the new behavior is correct anymore

Thinking about it more, I am taking back what I wrote yesterday about "getting it". It is a matter of semantics, the difference between .stubs and .expects. If I had:

MyService
  .expects(:expensive_method) # <--- this
  .yields("a value")
  .returns("a value")
  .times(3)

Then indeed, I am making an explicit expectation. I specify exactly the invocation, and I would want to get that LocalJumpError if no block was passed.

But I am using .stubs. I am not setting an expectation, I just want to get the real method out of the way. As the doc says: "It may be called any number of times with any parameters"... It feels natural to add "with or without a block" to that list.

So I am going back to: maybe the change in #382 needs to be reevaluated. Would it make sense to have the old behavior when using .stubs, and the new one when doing .expects?

[floehopper]

@yemartin Just to let you know, I've released v1.10.2 which includes a new configuration option (reinstate_undocumented_behaviour_from_v1_9). It does mean that for the moment you will see deprecation warnings, but hopefully (as per the more recent conversation on this issue) we can find a way forward for the next release which will mean you can avoid needing the deprecated behaviour. I hope that all makes sense.

[floehopper]

There's some more detailed documentation here.

[floehopper]

@nitishr, @yemartin Thanks for your input. I'm currently spiking on a slight variation of what I mentioned earlier. The variation is adding new parameter matchers to allow matching on the presence or absence of a block, e.g. foo.expects(:bar).with(no_block) or foo.expects(:bar).with(any_block). I think doing this is helping me understand what does and does not make sense for Mocha's API, so I'll respond to your points properly when I've got my head round it all a bit more. I hope that's OK.

[floehopper]

I'm currently spiking on a slight variation of what I mentioned earlier. The variation is adding new parameter matchers to allow matching on the presence or absence of a block, e.g. foo.expects(:bar).with(no_block) or foo.expects(:bar).with(any_block).

I'm not sure this ☝️ is going to fly, because it interferes too much with standard parameter matchers.

So I'm now exploring the with_block_given approach.

[floehopper]

I have 2 contradicting lines of reasoning about this, and I'm still trying to make up my mind about which one leads to a more logical and coherent (and ideally simpler) outcome.

On the one hand, in response to

[...] my actual broken test is a high level integration test. The method is called many times during the test, so hardcoding that exact sequence of invocations as stubs would be quite tedious.

I'm not sure mocha is the the right tool for high level integration tests with an unknown/unpredictable/uncontrollable sequence of method calls. After all, we don't allow conditional return values through the returns specification - instead requiring an exact sequence of return values if you did want the returned value to be different for different calls. Not sure that yielded values - or yielding - should be conditional by that same logic. At a first glance at least, this would seem to take us dangerously close to mocha being used for faking, which it's explicity (at this point) not meant for.

On the other hand, I recognize we do support parameter matchers to specify different responses to different parameters. Perhaps you could consider blocks (or the presence or absence of them) to be a kind of parameter matching, thereby matching different expectations in each case. I guess @floehopper's suggestion/proposal follows that reasoning.

@nitishr I think both your lines of reasoning are legitimate. Mocha's sweet spot definitely isn't high-level integration/acceptance tests. Having said that from a pragmatic point-of-view, we have to accept that many people have been using it in this way for years. I think this is why I'm drawn towards the idea of enhancing the existing parameter matching to include any block passed in. This doesn't seem inconsistent with the way Mocha currently works and would support @yemartin's use case.

[floehopper]

Thank you for the input @nitishr. I will try to give a bit more details about what we are actually doing but it will take me some time to summarize without going into unnecessary details. So first, let me clear two important point:

1. Our return / yielded values are the same on every call, so the use case should fit Mocha

After all, we don't allow conditional return values through the returns specification - instead requiring an exact sequence of return values if you did want the returned value to be different for different calls.

I made the yielded and returned values different in the code sample for clarity, but in our actual test, we return / yield the same value, on every call. It is only the sequence of invocations that is intricate, and maybe even unpredictable. And looking at the Mock#stubs documentation (emphasis mine):

Adds an expectation that the specified method may be called any number of times with any parameters.

This allows for an unpredictable sequence of invocations, so I believe our use case fits Mocha well.

2. I am not sure the new behavior is correct anymore

Thinking about it more, I am taking back what I wrote yesterday about "getting it". It is a matter of semantics, the difference between .stubs and .expects. If I had:

MyService
  .expects(:expensive_method) # <--- this
  .yields("a value")
  .returns("a value")
  .times(3)

Then indeed, I am making an explicit expectation. I specify exactly the invocation, and I would want to get that LocalJumpError if no block was passed.

But I am using .stubs. I am not setting an expectation, I just want to get the real method out of the way. As the doc says: "It may be called any number of times with any parameters"... It feels natural to add "with or without a block" to that list.

So I am going back to: maybe the change in #382 needs to be reevaluated. Would it make sense to have the old behavior when using .stubs, and the new one when doing .expects?

@yemartin

I think there's something in what you're saying, although I think it's more to do with the default parameter matching (ParameterMatchers#any_parameters) for an expectation rather than the difference between ObjectMethod#expects and ObjectMethod#stubs. i.e. I can see an argument to say that the default parameter matching should continue to include both the case where a block is not given and a where a block is given (which it does implicitly at the moment). However, as you rightly point out, this is then somewhat in conflict with the change in #382. This is because, unlike returning a value from a method, yielding values to a block can fail (with a LocalJumpError if no block is given). I need to give this some more thought.

[floehopper]

@yemartin Mocha v1.11.x introduces the Expectation#with_block_given and #with_no_block_given methods. Hopefully this will allow you to upgrade to the latest Mocha without any deprecation warnings. I'm going to close this issue, but feel free to re-open it if the same issue recurs. And don't hesitate to ask for help if you get stuck.