Make have_received complain if passed a block #364

timcowlishaw · 2013-07-12T15:55:07Z

I rather stupidly assumed I could use a block passed to have_received to make
assertions about the arguments passed in a method call, in the same way that is
possible with the 'receive' matcher. Ordinarily I wouldn't think such a mistake
would warrant being guarded against in the test library itself, however, in this
case the test silently passes, rather than erroring out. This patch makes
have_received raise an exception if passed a block argument to make it clear
that matching against arguments to the method call in this way is not possible.

I rather stupidly assumed I could use a block passed to have_received to make assertions about the arguments passed in a method call, in the same way that is possible with the 'receive' matcher. Ordinarily I wouldn't think such a mistake would warrant being guarded against in the test library itself, however, in this case the test silently passes, rather than erroring out. This patch makes have_received raise an exception if passed a block argument to make it clear that matching against arguments to the method call in this way is not possible.

fables-tales · 2013-07-12T15:56:15Z

spec/rspec/mocks/matchers/have_received_spec.rb

+        it "raises an exception when a block is used to match the arguments" do
+          dbl = double_with_met_expectation(:expected_method)
+          expect {
+            expect(dbl).to have_received(:expected_method) { |argument|


@timcowlishaw this can be reduced to one line:

`expect(dbl).to have_received(:expected_method) {}

fables-tales · 2013-07-12T15:57:37Z

@timcowlishaw thanks!

BTW other RSpec superfriends: I worked with tim and gave him some pointers on this one.

coveralls · 2013-07-12T16:01:13Z

Coverage remained the same when pulling fed03c9 on timcowlishaw:feature-have-received-complains-if-passed-a-block into 19d39b7 on rspec:master.

coveralls · 2013-07-12T16:07:18Z

Coverage decreased (-0%) when pulling 49c9f09 on timcowlishaw:feature-have-received-complains-if-passed-a-block into 19d39b7 on rspec:master.

JonRowe · 2013-07-12T23:51:25Z

spec/rspec/mocks/matchers/have_received_spec.rb

+          dbl = double_with_met_expectation(:expected_method)
+          expect {
+            expect(dbl).to have_received(:expected_method) { }
+          }.to raise_error(/have_received matcher does not take a block argument/)


Given that you are using caller[0] here to specify the location causing the error, it'd be nice if you included that in the error regexp, to assert that it's always the right line. You can use __FILE__ and __LINE__ to avoid hardcoding the file/line.

I realised this is dumb, because it'll happen in the backtrace. Sorry @timcowlishaw.

I think this might make more sense as a warning with caller(0). What do you think @JonRowe ?

I just think we need to spec which line these come from, as we've had a few instances where we've messed it up, or the ruby implementation has a different idea about caller.

what do you think about warning versus raising though? We get the backtrace for free with a raise but it stops execution. I think a warning is a better solution.

Yep, agreed on both counts. Assuming you wouldn't rather a PR allowing has_received to take a block to match args (as proposed below), I'll add a spec and turn this into a warning!

Thanks,

Tim

Do this, we'll merge it (maybe into 2-14-maintenance if others agree too), and if you feel like making it take a block to match args as another PR that'd be awesome :)

Great stuff. Will sort this out tomorrow!

myronmarston · 2013-07-13T01:21:32Z

I wonder if it would make sense for it to accept and use a block somehow. @timcowlishaw -- can you give us an example of how you were trying to use the block? It might be worth supporting.

JonRowe · 2013-07-13T10:22:08Z

My first instinct was "what the, it can't take a block?" so I'd be in favour of allowing it to have a block.

timcowlishaw · 2013-07-13T11:10:40Z

@JonRowe @myronmarston - I tend to agree here actually. The situation I had was something like this

    let(:missile_silo) { double(:missile_silo, :fire_missiles! => true) }

    it "calls the missile silo with a user authorized to fire missiles" do
      missile_command = MissileCommand.new(missile_silo)
      missile_command.start_war!
      expect(missile_silo).to have_received(:fire_missiles!) do |authorizing_user|
        expect(authorizing_user.rank).to eq(:commander)
      end
    end

Now, this seems like a pretty reasonable behaviour to me, and is in line with what receive does - allow expectations to be set against arguments within the block, and fail the test if any of them fail on any call to the doubled method. However, it can never work exactly like the receive version, as the return value of the block is used as the return value to the call to the mocked method when receive is used, which is obviously impossible with spies as the call to the block happens after the call to the mocked method has returned.

Subject to this caveat though, I for one would be very pleased to be able to use a block with have_received in this way. Would you like me to prepare a new PR with that behaviour?

Cheers,

Tim

JonRowe · 2013-07-13T11:16:42Z

I'd be in favour of cleaning this up and merging into 2-14-maintenance, and then if another PR comes in supporting using a block with have_received that can go into 3. (I'd also be in favour of that)

timcowlishaw · 2013-07-13T11:29:43Z

Cool, I will do both of those things. Thank you!

myronmarston · 2013-07-13T14:20:49Z

Now, this seems like a pretty reasonable behaviour to me, and is in line with what receive does - allow expectations to be set against arguments within the block, and fail the test if any of them fail on any call to the doubled method. However, it can never work exactly like the receive version, as the return value of the block is used as the return value to the call to the mocked method when receive is used, which is obviously impossible with spies as the call to the block happens after the call to the mocked method has returned.

I think your use case is reasonable. The reason we didn't already support this is because for receive, the block is used as the implementation of the method when the message is received. As the implementation, you can set an expectation like you're trying to do, but that's just one of many things you can do.

Anyhow, I think this is kinda like the raise_error matcher: it takes a block so that you can set further expectations on the exception.

I'd be in favour of cleaning this up and merging into 2-14-maintenance, and then if another PR comes in supporting using a block with have_received that can go into 3. (I'd also be in favour of that)

I'm not sure I agree. If we're going to add support for blocks, I don't want to release code that tells users that blocks aren't supported, as that'll create confusion. I'm also not sure if this kind of fix belongs in a patch release; patch releases are meant to be bug fixes only, and this feels like a new feature to me. @alindeman, what do you think?

alindeman · 2013-07-13T14:38:34Z

I agree that we should support the example shown above. We currently document the ability to set custom expectations on fake implementations in the README, so I think it makes sense to unify it for spies too.

Really nice example, @timcowlishaw.

I think this is simply a new feature, but not a bug. I think the feature should go into 3.0 while we leave everything as-is in 2.14 and 2.99.

JonRowe · 2013-07-14T03:51:02Z

I think that any implementation using the block is a new feature, but it'd be nice to warn that it doesn't do anything in 2-14-maintenance, maybe change the message just to say that the block isn't currently used?

…or file / line number

coveralls · 2013-07-14T10:12:51Z

Coverage increased (+0%) when pulling d6ed640 on timcowlishaw:feature-have-received-complains-if-passed-a-block into 19d39b7 on rspec:master.

timcowlishaw · 2013-07-14T10:13:51Z

Right guys, I've amended this to warn rather than raise when a block has been passed, and have amended the regex in the spec to test for the presence of the filename and line number. I'll prepare a second PR which takes a block and yields the arguments for further assertions, and will leave it up to you whether or not you want to merge either, or both!

Thanks a lot,

Tim

JonRowe · 2013-07-14T10:28:30Z

spec/rspec/mocks/matchers/have_received_spec.rb

+        it "warns when a block is used to match the arguments" do
+          dbl = double_with_met_expectation(:expected_method)
+          self.stub(:warn => nil)
+          expect(dbl).to have_received(:expected_method) { }; line = __LINE__


nice hack for avoid calculating the __LINE__ offset :)

fables-tales · 2013-07-18T11:02:38Z

@timcowlishaw thinks this can be closed because we decided not to merge the warning.

fables-tales reviewed Jul 12, 2013
View reviewed changes

Change exception msg per @samphippen's suggestion

49c9f09

JonRowe reviewed Jul 12, 2013
View reviewed changes

warn rather than raise when block passed to have_received. Add spec f…

d6ed640

…or file / line number

JonRowe reviewed Jul 14, 2013
View reviewed changes

fables-tales closed this Jul 18, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make have_received complain if passed a block #364

Make have_received complain if passed a block #364

timcowlishaw commented Jul 12, 2013

fables-tales Jul 12, 2013

fables-tales commented Jul 12, 2013

coveralls commented Jul 12, 2013

coveralls commented Jul 12, 2013

JonRowe Jul 12, 2013

fables-tales Jul 13, 2013

fables-tales Jul 13, 2013

JonRowe Jul 13, 2013

fables-tales Jul 13, 2013

JonRowe Jul 13, 2013

timcowlishaw Jul 13, 2013

JonRowe Jul 13, 2013

timcowlishaw Jul 13, 2013

myronmarston commented Jul 13, 2013

JonRowe commented Jul 13, 2013

timcowlishaw commented Jul 13, 2013

JonRowe commented Jul 13, 2013

timcowlishaw commented Jul 13, 2013

myronmarston commented Jul 13, 2013

alindeman commented Jul 13, 2013

JonRowe commented Jul 14, 2013

coveralls commented Jul 14, 2013

timcowlishaw commented Jul 14, 2013

JonRowe Jul 14, 2013

fables-tales commented Jul 18, 2013

Make have_received complain if passed a block #364

Make have_received complain if passed a block #364

Conversation

timcowlishaw commented Jul 12, 2013

Choose a reason for hiding this comment

fables-tales commented Jul 12, 2013

coveralls commented Jul 12, 2013

coveralls commented Jul 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myronmarston commented Jul 13, 2013

JonRowe commented Jul 13, 2013

timcowlishaw commented Jul 13, 2013

JonRowe commented Jul 13, 2013

timcowlishaw commented Jul 13, 2013

myronmarston commented Jul 13, 2013

alindeman commented Jul 13, 2013

JonRowe commented Jul 14, 2013

coveralls commented Jul 14, 2013

timcowlishaw commented Jul 14, 2013

Choose a reason for hiding this comment

fables-tales commented Jul 18, 2013