rspec / rspec-mocks

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

Matchers missing compound methods #1298

Open eLod opened 4 years ago

eLod commented 4 years ago

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

Steps to reproduce

Try to chain compound methods onto have_received matcher.

Expected behavior

Should work.

Actual behavior

It raises NoMethodError.

pirj commented 4 years ago

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.:

  1. pass

    a.foo(1)
    a.foo(2)
  2. 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.
  3. same

    a.foo(2)
    a.foo(1)
    #        #<A:0x00007ffd7dc89da8> received :foo out of order
  4. 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 commented 4 years ago

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 commented 4 years ago

@eLod would you like to work on this improvement?

eLod commented 4 years ago

@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 commented 4 years ago

@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 https://github.com/rspec/rspec-mocks/issues/1298#issuecomment-553799720).

eLod commented 4 years ago

@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: #" (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 commented 4 years ago

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

eLod commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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.