deprecate current method dispatch behavior(jmock1)

[nitishr]

Display deprecation warning when adding an expectation if the expectation list
already contains any expectations. The intent is to change method dispatch
behavior to that of jMock v2 as suggested in #173 in a major version release.

[floehopper]

I notice that this deprecation warning will necessarily be raised quite pervasively in many project test suites.

Thinking about it a bit more, I'm not sure whether it makes sense to deprecate the old method dispatch behaviour, at least not until the new behaviour is available.

Realistically, I can imagine there will be a lot of projects around that won't want to re-write all their tests just to upgrade to the latest version of Mocha. So I'm not sure we'd ever want to completely remove the old behaviour.

I think we have a couple of options:

Option A

1. Add new behaviour behind a configuration option
2. Bump minor version

Option B

1. Add new behaviour behind a configuration option
2. Deprecate old behaviour and suggest using new behaviour
3. Bump major version
4. Make new behaviour the default
5. Retain old behaviour behind a configuration option

Do you have any thoughts on which approach we should take?

[floehopper]

Also stepping back a bit, I think the original issue was opened in relation to questions I was getting like the one in #171 back in 2013. I don't think I've had many/any questions like that since around that time, so I wonder whether it's really worthwhile making this change. @nitishr What do you think...?

It might be worth looking at the jMock release notes to understand their rationale for making the change.

[nitishr]

I couldn't find jMock release notes, other than this:

JMock 2.0.0 RC1 has been released. Version two brings many improvements over version 1.1.0, including a more convenient and readable API, better support for refactoring IDEs and support for multiple test frameworks.

But I found this in the current readme here

Expectations match in first-in, first-out order, so tests are easier to understand.

That's also the main (probably the sole) reason I think the change in method dispatch order is worth making.

The rationale behind the old jmock and current mocha dispatch order seems to be:

This scheme allows you to:

• Set up default stubs in your the setUp method of your test class and override some of those stubs in individual tests.
• Set up different "once" expectations for the same method with different action per invocation. However, it's better to use the onConsecutiveCalls method to do this, as described below.

However, I don't think that rationale is strong enough to justify providing what appears to be a conterintuitive dispatch order to me. I can't imagine someone who hasn't read the documentation not being surprised by that behavior, even if they might understand and make peace with after reading the documentation.

I suspect that's the reason jMock adopted the opposite and (to me) more intuitive order. I could try asking @sf105 or @npryce whether they might remember and be willing to share the reasoning, as well as the impact of completely switching off the old behavior with a major version release, since that's one of our concerns.

Thoughts?

[floehopper]

@nitishr Thanks for researching the jMock documentation. I'm continuing to think about this and ask colleagues what behaviour they find least surprising. To get another data point, I've just started investigating how RSpec mocks work, but I've run out of time. So far I've created an RSpec mocks version of this acceptance test:

RSpec.describe 'rspec-mocks method dispatch' do
  it 'finds latest matching expectation' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
  end

  it 'finds latest expectation which has not stopped matching' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).once.and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(1)
  end

  it 'keeps finding later stub and so never satisfies earlier expectation (expected to fail)' do
    mock = instance_double('mock')
    expect(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
  end

  it 'finds later expectation until it stops matching then find earlier stub' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    expect(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(1)
  end

  it 'finds latest expectation with range of expected invocation count which has not stopped matching' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).at_least(2).at_most(3).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(1)
  end
end

And I see the following output:

Failures:

  1) rspec-mocks method dispatch finds latest expectation which has not stopped matching
     Failure/Error: expect(mock.method).to eq(1)
     
       (InstanceDouble(mock) (anonymous)).method(no args)
           expected: 1 time with any arguments
           received: 2 times
     # ./spec/rspec_mocks_method_dispatch_spec.rb:16:in `block (2 levels) in <top (required)>'

  2) rspec-mocks method dispatch keeps finding later stub and so never satisfies earlier expectation (expected to fail)
     Failure/Error: expect(mock.method).to eq(2)
     
       expected: 2
            got: 1
     
       (compared using ==)
     # ./spec/rspec_mocks_method_dispatch_spec.rb:24:in `block (2 levels) in <top (required)>'

  3) rspec-mocks method dispatch finds latest expectation with range of expected invocation count which has not stopped matching
     Failure/Error: expect(mock.method).to eq(1)
     
       (InstanceDouble(mock) (anonymous)).method(no args)
           expected: at most 3 times with any arguments
           received: 4 times
     # ./spec/rspec_mocks_method_dispatch_spec.rb:45:in `block (2 levels) in <top (required)>'

Finished in 0.02512 seconds (files took 0.10137 seconds to load)
5 examples, 3 failures

Failed examples:

rspec ./spec/rspec_mocks_method_dispatch_spec.rb:11 # rspec-mocks method dispatch finds latest expectation which has not stopped matching
rspec ./spec/rspec_mocks_method_dispatch_spec.rb:20 # rspec-mocks method dispatch keeps finding later stub and so never satisfies earlier expectation (expected to fail)
rspec ./spec/rspec_mocks_method_dispatch_spec.rb:38 # rspec-mocks method dispatch finds latest expectation with range of expected invocation count which has not stopped matching

I think it would be good to think about this a bit more and ideally have a stronger motivation/explanation for making any changes.

[nitishr]

I've updated the rspec example as below:

RSpec.describe 'rspec-mocks method dispatch' do
  it 'finds latest matching allowance' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
  end

  it 'does not find earlier allowance even after a constrained later allowance is used up' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).once.and_return(2)
    expect(mock.method).to eq(2)
    expect { mock.method }.to raise_error(RSpec::Mocks::MockExpectationError)
  end

  it 'finds prior expectation before later allowance' do
    mock = instance_double('mock')
    expect(mock).to receive(:method).and_return(1)
    allow(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
  end

  it 'finds later expectation before prior allowance' do
    mock = instance_double('mock')
    allow(mock).to receive(:method).and_return(1)
    expect(mock).to receive(:method).and_return(2)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(1)
  end

  it 'finds prior expectation before later expecation' do
    mock = instance_double('mock')
    expect(mock).to receive(:method).and_return(1)
    expect(mock).to receive(:method).at_least(:once).and_return(2)
    expect(mock.method).to eq(1)
    expect(mock.method).to eq(2)
    expect(mock.method).to eq(2)
  end
end

And I see the following output:

rspec-mocks method dispatch
  finds latest matching allowance
  does not find earlier allowance even after a constrained later allowance is used up
  finds prior expectation before later allowance
  finds later expectation before prior allowance
  finds prior expectation before later expecation

Finished in 0.02154 seconds (files took 0.14757 seconds to load)
5 examples, 0 failures

(Note: while reviewing the tests, it'll help to remember that in rspec, when no invocation counts are specified, stubs default to any number of times, while mocks default to exactly once.)

So, it seems the rules are:

1. earlier allowances (stubs) are totally forgotten - only the last allowance 'survives'
2. earlier expectations (mocks) are matched before later expectations
3. expectations are matched before allowances (and rules 1 and 2 still hold)

They seem unnecessarily complicated to me. Not sure if they've been deliberaty designed or accidentally implemented.

[floehopper]

@nitishr

Thanks so much for investigating RSpec's behaviour so thoroughly - that's really helpful and interesting!

I'm afraid I still haven't had time to think about this in any detail.

Are you still thinking that reversing the current Mocha matching order (like you implemented in #395) is the "right" solution. Have you had a chance to ask any other Ruby-ists what they think?

Given it's a significant change, it would be good to canvas as much opinion as possible to make the behaviour as unsurprising as possible.