shouldNot receive: passes when the message is received

[chrisdevereux]

[[object should] receive:selector] is equivalent to [[object should] receive:selector withCount:1]

However, this means that [[object shouldNot] receive:selector] is equivalent to [[object shouldNot] receive:selector withCount:1]

This means that the matcher tests for the opposite of what it says it tests.

There seem to be 3 options for fixing this:

1. Redefine [[object should] receive:selector] to do [[object should] receive:selector withCountAtLeast:1]. Arguably, this is closer to what "should receive" means normally, but it's a breaking change.
2. Make the matcher behave differently depending on whether it's a should or shouldNot.
3. Deprecate receive: and replace it with receiveOnce: or something that makes things more explicit.

My preference would be for 1, but I thought I'd check what people think before starting anything.

[modocache]

This is a great bug. Thanks for the issue, @chrisdevereux!

RSpec takes the approach you describe in (2); it fails fast if the matcher is a negative clause. That is, the following spec:

describe Car do
  let(:car) { described_class.new }

  it 'should not receive honk' do
    car.should_not_receive(:honk) # or `expect(car).to_not receive(:honk)`
    car.honk
    car.honk
  end
end

Fails with the following message (notice the failure message says "received 1 time", not "2 times"):

  1) Car should not receive honk
     Failure/Error: car.honk
       (#<Car:0x007fc7b42e7290>).honk(no args)
           expected: 0 times with any arguments
           received: 1 time
     # ./car_spec.rb:11:in `block (2 levels) in <top (required)>'

I also initially thought (1) was better. However, if the user expects a fast-fail, but the message is sent twice and thus withCountAtLeast: passes, the test results in a bewildering false positive. I don't think most users will realize that "should not receive" means "should not receive at least once", which rephrased means "should receive two times or more"--I even had trouble typing that out.

I have yet to examine whether Kiwi can implement (2) easily, but if you'd like to take a stab at it, I definitely think that's the way to go.

[chrisdevereux]

Hey, thanks for the quick reply.

Not sure I understand this point:

I don't think most users will realize that "should not receive" means "should not receive at least once", which rephrased means "should receive two times or more"--I even had trouble typing that out.

If "shouldNot receive" means !(receivedCount >=1), then since the received count is unsigned, it's the same as receivedCount == 0, which is the expected behavior. Am I missing a case where this wouldn't be the behavior expected by the user?

I agree that the failure message would be a bit strange though. Maybe a better solution would be to go with the behavior described above, but move should/shouldNot into a separate class to the other shouldReceive... matchers and provide a more comprehensible failure message.

[chrisdevereux]

I'm assuming here that the behavior we want is that should receive matches when some nonzero number of messages is sent, while shouldNot receive matches when zero messages are sent.

Moving from the current behavior to that would cause currently passing tests to fail (in some case, desirably so, in some cases not), but I can't think of a case where it causes a false-positive.

[modocache]

Whoops, I think I got confused while writing my first comment. Indeed you're right; my argument about the false positive was flawed--my bad!

I'm assuming here that the behavior we want is that should receive matches when some nonzero number of messages is sent, while shouldNot receive matches when zero messages are sent.

Actually, the following spec fails in RSpec:

describe Car do
  let(:car) { described_class.new }

  it 'should receive honk' do
    car.should_receive(:honk) # or `expect(car).to receive(:honk)`
    car.honk
    car.honk
  end
end

The failure message indicates that the spec fails because should_receive only matches when the message is sent exactly once:

  1) Car should receive honk
     Failure/Error: car.should_receive(:honk) # or `expect(car).to receive(:honk)`
       (#<Car:0x007fccea94f278>).honk(any args)
           expected: 1 time with any arguments
           received: 2 times with any arguments
     # ./car_spec.rb:10:in `block (2 levels) in <top (required)>'

So if we wanted to emulate how RSpec behaves, we would not want should receive to match when a nonzero amount of messages are sent, but rather to match only when exactly one message is sent.

I think users generally expect Kiwi to behave like RSpec does (just search this repository for the string "RSpec" and GitHub will return plenty of issues pointing out discrepancies), so I think the best thing to do here is to change how shouldNot receive behaves, without changing the behavior of should receive.

[chrisdevereux]

Ahhh, got it now. Thanks!

[modocache]

I think the intent of method would be clearer if it was named -setWillEvaluateAgainstNegativeExpectation: or -setIsNegativeExpectation:.

[modocache]

Awesome! I pulled this down and confirmed it works exactly as expected. I think this will be good to merge after addressing a few comments I left above.

@supermarin @stepanhruda This fixes a bug in the receive: matcher, but the fix will change the behavior slightly. Namely, the following test, which passes in 2.2.4 and prior, will begin to fail:

NSObject *mock = [NSObject mock];
[[mock shouldNot] receive:@selector(description)];
[mock description];
[mock description];

Personally I think this can be released in a minor version like 2.2.5. Let me know if you feel differently, otherwise I'll hit the big green button.

[Panajev]

I would personally say that it is quite important that the sample test you quoted fails as it would be very counterintuitive otherwise.

[web [iPhone msg]]

On 23/mar/2014, at 07:41, Brian Gesiak notifications@github.com wrote:

Awesome! I pulled this down and confirmed it works exactly as expected. I think this will be good to merge after addressing a few comments I left above.

@supermarin @stepanhruda This fixes a bug in the receive: matcher, but the fix will change the behavior slightly. Namely, the following test, which passes in 2.2.4 and prior, will begin to fail:

NSObject *mock = [NSObject mock];
[[mock shouldNot] receive:@selector(description)];
[mock description];
[mock description];
Personally I think this can be released in a minor version like 2.2.5. Let me know if you feel differently, otherwise I'll hit the big green button.

�
Reply to this email directly or view it on GitHub.

[supermarin]

++ for merging after your comments are addressed.
Ship in 2.2.5

[chrisdevereux]

Argh, just spotted a problem:

This issue also applies to -receive:withArguments:, -receiveAndReturn: and so on. I'll add this behavior to them too.

[modocache]

Choice. Is this ready to be merged, @chrisdevereux? (Sorry for the delay, I don't get a notification when commits are added to PRs 😦)

[chrisdevereux]

Sorry, should have added a comment! I'm happy with it. I didn't add this behavior to the depracated NSInvocation-based matchers (would have required more dramatic changes to the KWShouldReceiveMatcher class and presumably they're going away soon anyway).

If that's a problem, I can take a look, otherwise go for it!

[modocache]

If anyone wants the same behavior for the deprecated matchers they can file an issue, or preferably a pull request. I'd rather fix the supported matchers as soon as possible.

Thanks for pointing out the issue, and then fixing it! 👍