Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
VitaliiLazebnyi opened this issue Jun 25, 2021 · 7 comments
Open

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

VitaliiLazebnyi opened this issue Jun 25, 2021 · 7 comments

Comments

@VitaliiLazebnyi
Copy link

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

  • Ruby version: ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux-gnu]
  • rspec-mocks version: rspec-mocks (3.7.0)

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
Copy link
Member

pirj commented Jun 25, 2021

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
Copy link
Author

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

@pirj
Copy link
Member

pirj commented Jun 25, 2021

Something like

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

? 🤔

@VitaliiLazebnyi
Copy link
Author

Yes, something like it.

@pirj
Copy link
Member

pirj commented Jun 28, 2021

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
Copy link
Member

pirj commented Jun 28, 2021

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants