Weird behaviour with .should_receive(...).at_least(0).times in 2.10.0

[lsegal]

This gist tells the story: https://gist.github.com/2592376 including output. The minimal reproduction code is (copied from gist for convenience):

require 'rspec'

describe 'at_least(n).times' do
  # Pass
  it 'RSpec 2.9.0+ allows at_least(N).times when N > 0' do
    mymock = mock
    mymock.should_receive(:run).at_least(1).times
    mymock.run
    mymock.run
  end

  # Fail
  it 'RSpec 2.9.0+ does *not* allow at_least(N).times when N = 0' do
    mymock = mock
    mymock.should_receive(:run).at_least(0).times
    mymock.run
  end

  # Pass
  it 'RSpec 2.10.0 allows at_least(N).times when N = 0 if .and_return is added' do
    mymock = mock
    mymock.should_receive(:run).at_least(0).times.and_return(true)
    mymock.run.should be_true
  end
end

Basically, in 2.9.0 (the last version I had tried this with), RSpec seemed to behave incorrectly if you passed in .at_least(0).times to a matcher. In 2.9.0 both specs with N=0 above will fail. 2.10.0 seems to fix the 3rd case when you have an .and_return clause, but that seems like a side-effect rather than an intentional fix. You still cannot do .at_least(0).times.

Oddly enough, my actual real world spec (see here) has a .and_return, and yet it worked in 2.9.0 but only started failing in 2.10.0, which completely contradicts the minimal case above. So something is going on behind the scenes here, but I assume if .at_least(0).times is fixed to work properly, the side effects will be solved as well.

edit: note that my gist runs the specs in ruby 1.9.3p125, but Travis shows the specs failing for all Rubies, in the real world code.

[dchelimsky]

That is odd, but I'd rather disallow 0 as an argument to at_least on the grounds that that is effectively a stub :)

[dchelimsky]

Excellent bug report, btw!

[lsegal]

The issue is that, as you can see from the real-world example, some cases are N>1, and others are N=0. I'd rather not have to handle a special case when N=0 manually if it's an integer value in the API, especially when this was working for us for quite a while (that specific test is ~3 years old).

[dchelimsky]

I'm confused. Seems to me that these should all pass if we're supporting at_least(0):

describe 'at_least(n).times' do
  example do
    mymock = mock
    mymock.should_receive(:run).at_least(0).times
    mymock.run
  end

  example do
    mymock = mock
    mymock.should_receive(:run).at_least(0).times { true }
    mymock.run.should be_true
  end

  example do
    mymock = mock
    mymock.should_receive(:run).at_least(0).times.and_return(true)
    mymock.run.should be_true
  end
end

What I'm seeing is that in 2.9 all 3 fail, but in 2.10 only the first 2 fail. So 2.9 was completely broken and 2.10 is partially fixed.

So it seems like your real world example should have been failing until 2.10 "fixed" the .and_return case.

[dchelimsky]

@lsegal I fixed what I perceive to be the bug here, which is that at_least(0) failed in some cases when a message was received. Unless I'm misunderstanding your request, this seems like the opposite of what you're looking for, but I'd argue this is the correct behavior if we're going to continue to support at_least(0). WDYT?

[dchelimsky]

Actually - that was a bit naive - with this change it still fails if you say at_least(0).times.and_return(value) and it is never called. I'm going to fix that, but thinking more and more that 0 should simply not be allowed.

[lsegal]

Basically, I want the behaviour from 2.9.0 and prior versions :) except when writing up the report, I wasn't aware that the behaviour was always so finnicky. Basically, we were getting lucky this whole time, or maybe the functionality started degrading at a previous version, I can't tell, but either way, we need at_least(0).times to work, with or without .and_return. Fixing that should fix the issue we're having.

FYI I'm relatively okay with changing at_least(0), but if you want to make a change to the API, it should be done in a 3.x.x release, because that would be an incompatible behaviour. Then again, I'd argue that at_least(0) should stick around, because it's specifically useful in my case (where the number of times is configurable down to 0) and makes sense in the context of the API even though it may duplicate functionality that exists elsewhere (like any_number_of_times or a stub).

[lsegal]

FYI I just tested this change and it fixes our issue. However, one failing test remains (that didn't fail with 2.9.0) in a similar way (we're using .should_not_receive here). I will open an issue for this in a few.

[dchelimsky]

FYI I'm relatively okay with changing at_least(0), but if you want to make a change to the API, it should be done in a 3.x.x release, because that would be an incompatible behavior.

Agreed. If we do it there will be a deprecation warning on at_least(0) in a 2.x release, and then it would raise an error in 3.x.

[dchelimsky]

@lsegal I'm going to do a 2.10.1 release of rspec-mocks and rspec-rails over the weekend. If you get me the issue today I'll get it fixed for that release. Thx.

[lsegal]

Ah, update: the second issue was due to calling .and_return on .should_not_receive, probably similar to .at_least(0).times.and_return. Admittedly, this spec is wrong, so I'm not going to open an issue for it, but I'd point out two things and let you decide whether to fix:

1. It is a backward incompatible change in a minor revision. Either that or it is a regression, since it was working in 2.9.0. Was this an explicit change?
2. Even though it's stylistically wrong and logically impossible (don't really know how this code got in there), I don't think .should_not_receive should care about what other modifiers are appended to the mock. Does .and_return mean that the mock must return something?

The reproducing code is as follows:

require 'rspec'
describe 'should_not_receive' do
  it { mock.should_not_receive(:run).and_return(true) }
end

I'll let you decide what to do. FYI we just changed this line in our specs so we don't have the issue anymore.