rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 358 forks source link

Impossible to check that method is executed in a range times. #1426

Open VitaliiLazebnyi opened 3 years ago

VitaliiLazebnyi commented 3 years ago

Subject of the issue

According to the documentation methods, at_least and at_most can be chained to have range check: method is called between N and M times. (at least N times and not more then M times). But rspec-mocks actually checks the last condition and ignores previous ones.

Your environment

Steps to reproduce

  it "doesn't support double checks" do
    tmp = double('Test Variable')
    expect(tmp).to receive(:call).and_return(nil).at_least(1).at_most(2).times
  end

  it "doesn't support double checks" do
    tmp = double('Test Variable')
    expect(tmp).to receive(:call).and_return(nil).at_most(2).at_least(1).times
    tmp.call
    tmp.call
    tmp.call
  end

Expected behavior

Both tests should fail since one of the conditions is not followed.

Actual behavior

Both tests pass since the condition, which is at the end, overwrites the previous one.

pirj commented 3 years ago

According to the documentation methods, at_least and at_most can be chained to have range check

I couldn't find this apart from self, to support further chaining. Surely, this is confusing, as the implementation disregards the previously set constraint and overwrites it.

What I would suggest:

  1. Untangle at_least/at_most/exactly by not using a common expected_received_count.
  2. Make exactly incompatible with the other two, add a failure message.
  3. Restrict using at_least/at_most more than once.

We can only afford 2 & 3 in a major version, but as we're nearing 4.0, it's possible to do so in 4-0-dev branch. The main branch would need to print a deprecation message when exactly used with at_least/at_most, or the latter two are set more than once.

I'd suggest taking a look at https://github.com/rspec/rspec-expectations/blob/43bf64b01f8356979ffbc373b2e81d2ab1389b29/lib/rspec/matchers/built_in/count_expectation.rb#L103 for inspiration.

Would you like to hack on it?

VitaliiLazebnyi commented 3 years ago

Is it acceptable to have 2+3+add method .in_range(from..to)?

pirj commented 3 years ago

Something like

expect(tmp).to receive(:call).and_return(nil).in_range(1..2).times

? 🤔

VitaliiLazebnyi commented 3 years ago

Yes, something like it.

pirj commented 3 years ago

is it acceptable to have 2+3+add method .in_range(from..to)?

Shortly: no.

The introduction of this new method is orthogonal to the fixes, and can be done separately. Adding a new method that would attempt to replace the existing malfunctioning/deceptive at_least/at_most is not a good option. I'd suggest addressing the problems we have now and introduce the new method after.

pirj commented 3 years ago

There's one more issue I could spot:

    r = double
    expect(r).to receive(:foo).at_most(:once).and_return(1, 2)
    r.foo
    r.foo

surprisingly, this passes.

yaroslavrick commented 9 months ago

This way it works for me to test how many times method is called:

 allow_any_instance_of(ClassName).to receive(:your_method)

 allow(ClassName).to receive(:find).and_return(your_instance) # if you need to test #update action
 put your_route_url(your_instance, subdomain: 'your_subdomain', host: '127.0.0.1.xip.io', type: 'your_type'), params: params # if you need to test #update action

 expect(your_instance).to have_received(:your_method).once