-
-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
return what the block returned when using yield #493
Comments
/cc @grosser |
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. |
ah yes, #227 is what we had in mind |
👋 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 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. |
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.
It would be helpful if you have any realistic examples which could help me understand what you're trying to do. |
I've put together an example that is much closer to where I have (most recently) encountered this issue: You can run 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:
|
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. |
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 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 If I simply change |
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 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 Also #227 suggests that a call to 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. |
Ah, yes. I am conflating two things:
Occasionally these are the same, but not always, and we should allow support for both of those things. Attacking this problem by defining the |
Just brainstorming here but is there some interface on
Perhaps |
@collinsauve Thanks for your replies.
I agree that these two things are being conflated.
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.
I don't think I'd go so far as saying that using something like the 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?
I'm not quite sure what you're getting at here.
You're right that I wouldn't want to overload In any case, I'm a bit unsure about adding an assertion, because there isn't really any precedent for this in the Anyway, this is all very useful food for thought. |
I came across this issue when searching for a way to return the value that the block returns, when using I would like to see this implemented in mocha, but for what it's worth I was able to add rspec-mocks to my Rails + Minitest project, and get it working with Mocha & Minitest. Ideally, it would be implemented in mocha so I wouldn't need rspec-mocks. Minimal example code under test: class ServiceWrapper
def self.save(key, value)
return Service.get_conn do |conn|
# returns true if successful
conn.save(key, value)
end
end
end Example test code with rspec-mocks: fake_service = FakeService.new
allow(Service).to(receive(:get_conn).and_wrap_original) do |_original_method, *_args, &block|
fake_service.with do |conn|
block.call(conn)
end
end
result = ServiceWrapper.save('abc', 123)
assert(result) I had to inherit from this base class: class RspecMockActiveSupportTestCase < ActiveSupport::TestCase
include ::RSpec::Mocks::ExampleMethods
def before_setup
::RSpec::Mocks.setup
super
end
def after_teardown
super
::RSpec::Mocks.verify
ensure
::RSpec::Mocks.teardown
end
end And in require 'minitest'
require 'rails/test_help'
require 'minitest/autorun'
require 'mocha/minitest'
require 'rspec/mocks' # must be required after mocha/minitest, to avoid conflicts |
@ricksuggs Thanks for posting. It's interesting to see how you've got mocha & rspec-mocks working alongside each other. I have to confess I'm slightly confused by your mocking example - can you edit your comment to add the code under test (or at least a simplified version) to make things clearer? For example, I can't see what the |
@floehopper Yes, I've updated the comment to show an example scenario where I need to use this. My real example is more complicated, so I've simplified it to be a minimal example. |
@ricksuggs Should |
No, it should have been |
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.
The text was updated successfully, but these errors were encountered: