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

Matchers missing compound methods #1298

Open
eLod opened this issue Nov 14, 2019 · 10 comments
Open

Matchers missing compound methods #1298

eLod opened this issue Nov 14, 2019 · 10 comments

Comments

@eLod
Copy link

eLod commented Nov 14, 2019

Subject of the issue

I was trying to use rspec expectations' compound feature as in expect().to have_received().and.have_received() (actually for using with .ordered), but the and method is missing from HaveReceived matcher (and i guess from the others too). Including RSpec::Matchers::Composable into the matcher solves the problem (though the error may be somewhat misleading, as for example if the argument expectations fail it still complains about calls out of order).

Your environment

  • Ruby version: 2.6.4
  • rspec-mocks version: 3.8.1

Steps to reproduce

Try to chain compound methods onto have_received matcher.

Expected behavior

Should work.

Actual behavior

It raises NoMethodError.

@pirj
Copy link
Member

pirj commented Nov 14, 2019

Good catch on have_received/receive not being composable.

Reproduction example with the difference between non-composable and composable expectations:

RSpec.describe 'have_received' do
  class A
    def foo(x)
    end
  end

  subject(:a) { A.new }

  it 'fails with a proper error message' do
    allow(a).to receive(:foo).with(1)
    allow(a).to receive(:foo).with(2)
    # action here
    expect(a).to have_received(:foo).with(1).ordered
    expect(a).to have_received(:foo).with(2).ordered
  end

  it 'fails with a proper error message when chained' do
    allow(a).to receive(:foo).with(1)
    allow(a).to receive(:foo).with(2)
    # action here
    expect(a)
      .to have_received(:foo).with(1).ordered
      .and have_received(:foo).with(2).ordered
  end
end

Depending on the order of the calls in action, e.g.:
0. pass

    a.foo(1)
    a.foo(2)
  1. same
    a.foo(1)
    a.foo(3)
 #      #<A:0x00007fcf272bb918> received :foo with unexpected arguments
 #        expected: (2)
 #             got: (3)
 #       Please stub a default value first if message might be received with other args as well.
  1. same
    a.foo(2)
    a.foo(1)
#        #<A:0x00007ffd7dc89da8> received :foo out of order
  1. different output
    a.foo(1)
#      #<A:0x00007ffaa4a836e8> received :foo with unexpected arguments
#         expected: (2)
#              got: (1)
#
# vs
#
#       #<A:0x00007ffaa3a9e340> received :foo out of order

In order for composability to work,

        include RSpec::Matchers::Composable

should be added to lib/rspec/mocks/matchers/receive.rb and lib/rspec/mocks/matchers/have_received.rb (optionally for receive_messages and receive_message_chain).

One spec in the suite fails, supposedly due to an issued warning.

@JonRowe
Copy link
Member

JonRowe commented Nov 14, 2019

We'd need to check that the module is defined before including it, as rspec-mocks is standalone from rspec-expectations however we also to fix that error, its occurring because ordered is not being recognised correctly.

@pirj
Copy link
Member

pirj commented Nov 15, 2019

@eLod would you like to work on this improvement?

@eLod
Copy link
Author

eLod commented Nov 16, 2019

@pirj sure, any pointers, recommendations? i get @JonRowe saying we should check if the module is defined before including it, but not sure about the second part "ordered is not being recognised correctly".

@pirj
Copy link
Member

pirj commented Nov 16, 2019

@eLod incorrect message (received :foo with unexpected arguments) is for non-compound variant, so it's a problem that we already have.
Compound expectation does not have this problem, and the failure message is more logical (received :foo out of order).

So I'd say go ahead and add that new feature, while incorrect message should be dealt with separately. Appreciate if you could file a ticket (with an example from case 3 from #1298 (comment)).

@eLod
Copy link
Author

eLod commented Nov 18, 2019

@pirj i started to work on it, but i'm having trouble writing specs or features, getting "only the receive, have_received and receive_messages matchers are supported with allow(...).to, but you have provided: #RSpec::Matchers::BuiltIn::Compound::And:..." (which i can't reproduce in a real project suite) originating from here. Am i supposed to change that .delegate_to and .delegate_not_to functionality or missing some setup/configuration step?

edit: sorry for the confusion, i think i've realised the compound functionality should only apply to .expect (not .allow), which does not raise the error

@pirj
Copy link
Member

pirj commented Nov 18, 2019

@eLod hm. Can you please push your work in progress? Let's figure it out together.

@eLod
Copy link
Author

eLod commented Nov 18, 2019

@pirj see my edit, i was trying to do allow().to receive(:foo).and(receive(:bar)) which is simply not supported, once i've realised that i was able to progress. working on the specs/features now, i will push once it's in an acceptable shape.

eLod added a commit to eLod/rspec-mocks that referenced this issue Nov 18, 2019
@eLod
Copy link
Author

eLod commented Nov 18, 2019

@pirj pushed changes, not sure about the shared examples, a bit complex on how the compound expectation is built and verified. also there are examples failing, all because of .or not working as expected (or i am missing something), e.g. not passing when only the second message is received. do you think it should be part of this feature (haven't investigated what is causing the problem)?

(edit: typo)
edit2: PR #1299

@eLod
Copy link
Author

eLod commented Nov 18, 2019

i am guessing it is because RSpec::Matchers::BuiltIn::Or.match simply calls matcher_1_matches? || matcher_2_matches? here, so the first receive (or have_received) gets triggered which causes the error.

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