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

Spies do not support ordering with interleaved calls #916

Open myronmarston opened 9 years ago

myronmarston commented 9 years ago
RSpec.describe "Specifying ordering for calls that are interleaved with each other" do
  it 'fails when using `have_received`' do
    dbl = spy

    dbl.one
    dbl.one
    dbl.two
    dbl.one
    dbl.two

    expect(dbl).to have_received(:one).twice.ordered
    expect(dbl).to have_received(:two).once.ordered
    expect(dbl).to have_received(:one).once.ordered
    expect(dbl).to have_received(:two).once.ordered
  end

  it 'passes when using `receive`' do
    dbl = double

    expect(dbl).to receive(:one).twice.ordered
    expect(dbl).to receive(:two).once.ordered
    expect(dbl).to receive(:one).once.ordered
    expect(dbl).to receive(:two).once.ordered

    dbl.one
    dbl.one
    dbl.two
    dbl.one
    dbl.two
  end
end

The second spec passes, but the first fails:

Specifying ordering for calls that are interleaved with each other
  fails when using `have_received` (FAILED - 1)
  passes when using `receive`

Failures:

  1) Specifying ordering for calls that are interleaved with each other fails when using `have_received`
     Failure/Error: expect(dbl).to have_received(:one).twice.ordered
       (Double).one(*(any args))
           expected: 2 times with any arguments
           received: 3 times with any arguments

I originally put a failing spec in #881 but don't have the time to fix it so I figured I'd convert it into an issue in the hope that someone else would pick it up.

johnceh commented 9 years ago

@myronmarston Potentially found another scenario or separate issue using a similar test.

it 'fails when using `have_received` with unexpected message' do
  dbl = spy

  dbl.one
  dbl.two

  expect {
    expect(dbl).to have_received(:one).once.ordered
  }.to raise_error(/received unexpected message :two/m)
end

it 'passes when using `receive` with unexpected message' do
  expect {
    dbl = double

    expect(dbl).to receive(:one).once.ordered

    dbl.one
    dbl.two
    }.to raise_error(/received unexpected message :two/m)
end

Second spec passes, but the first fails

Failures:

  1) RSpec::Mocks::Matchers::HaveReceived expect(...).to have_received ordered when used on individually allowed messages fails when using `have_received` with unexpected message
     Failure/Error: expect {
       expected Exception with message matching /received unexpected message :two/m but nothing was raised

I came across this after my pull request for this issue, and it seems similar enough to where it might be included within scope. Doesn't have to do with interleaved calls though, so I could also see it being it's own issue as well. Thoughts?

myronmarston commented 9 years ago

That's expected behavior. A double, by default, is strict -- that is, it only allows messages that you've specifically configured it to allow (via expect or allow...to receive...). As such, in the 2nd example, it should raise an error when two is received.

Strictness works well as the default behavior for a double, particular when you use ahead-of-time message expectations. It doesn't work as well when you want to use a double as a spy and assert that the message was received after-the-fact. Since a double will raise when it receives one (or any message), you have to configure it to allow that message before hand, and then you can assert it was received afterwards:

dbl  = double
allow(dbl).to receive(:one)

dbl.one

expect(dbl).to have_received(:one)

...but that requires you to specify the received message twice (once before, and once after), which seems smelly compared with just doing it once, before hand:

dbl = double
expect(dbl).to receive(:one)

dbl.one

Doubles can be configured to be loose instead of strict by calling as_null_object on them; that has the effect of automatically stubbing and spying on all messages, without the need to explicitly configure each and every one. spy is simply a more convenient form of double.as_null_object, allowing you to assert a message was received after-the-fact without needing to configure the double to allow that message beforehand.

Thus, the spy in your example should allow the two message. There's no reason the spy should raise an error about it just because the user hasn't specified that they expect that message to have been received. And actually, they might specify such a thing on a subsequent line:

expect(dbl).to have_received(:one).once.ordered
expect(dbl).to have_received(:two).once.ordered

...so it would be very counterproductive for the first line to raise an error about two. Spies would basically be unusable if we did that.

Does that help?

johnceh commented 9 years ago

Yes, that makes a lot of sense now. Haven't used spies enough to have picked up on that part of it's behavior, so thanks for the explanation. Once less thing for me to fix in the PR, so that's good news :)

johnceh commented 9 years ago

I could use some input whenever you get a chance @myronmarston, @JonRowe, or anybody else that would know how these tests are expected to respond.

I had my first pull request for this issue passing all the tests (minus 1 cucumber), but it's behavior was modeled closer to the strict vs loose since I made the wrong assumptions. I was hoping I might be able to get some direction on what would be expected on the examples below so I don't make the same mistake twice. Only the first one passes, and I'm not sure what the desired behavior of the other four would be. Much appreciated.

          context 'ordered spies' do
            # 1 PASSES
            it 'should pass when a spy receives unexpected messages before or after itself' do
              dbl = spy
              dbl.pre_one
              dbl.one
              dbl.post_one

              expect(dbl).to have_received(:one).once.ordered
            end

            # 2 FAILS
            it 'should pass when it receives unexpected messages of itself' do
              dbl = spy
              dbl.one
              dbl.one

              expect(dbl).to have_received(:one).once.ordered
            end

            # 3 FAILS
            it 'should pass when it receives messages of itself separated' do
              dbl = spy
              dbl.one
              dbl.between_one
              dbl.one

              expect(dbl).to have_received(:one).once.ordered
              expect(dbl).to have_received(:one).once.ordered
            end

            # 4 FAILS
            it 'should pass when it receives messages of itself together' do
              dbl = spy
              dbl.one
              dbl.one

              expect(dbl).to have_received(:one).once.ordered
              expect(dbl).to have_received(:one).once.ordered
            end

            # 5 FAILS
            it 'should it pass when in order that matches expectation order,despite pre/post :ones ' do
              dbl = spy
              dbl.one
              dbl.two
              dbl.one
              dbl.three
              dbl.one

              expect(dbl).to have_received(:two).once.ordered
              expect(dbl).to have_received(:one).once.ordered
              expect(dbl).to have_received(:three).once.ordered
            end
          end
myronmarston commented 9 years ago

The strict vs loose thing has to do with the type of double (normal vs as_null_object) and not the use of ahead-of-time receive vs after-the-fact have_received...although they are related, in that after-the-fact have_received can't be used with a strict double unless you explicitly allow the message first.

Still, I'd say that the behavior of receive is a good guide here. I converted each of these examples into receive equivalents:

RSpec.describe do
  # 1 PASSES
  it 'should pass when a spy receives unexpected messages before or after itself' do
    dbl = spy
    dbl.pre_one
    dbl.one
    dbl.post_one

    expect(dbl).to have_received(:one).once.ordered
  end

  it 'should pass when a spy receives unexpected messages before or after itself' do
    dbl = spy
    expect(dbl).to receive(:one).once.ordered

    dbl.pre_one
    dbl.one
    dbl.post_one
  end
end

These both pass, and both should pass.

RSpec.describe do
  # 2 FAILS
  it 'should pass when it receives unexpected messages of itself' do
    dbl = spy
    dbl.one
    dbl.one

    expect(dbl).to have_received(:one).once.ordered
  end

  it 'should pass when it receives unexpected messages of itself' do
    dbl = spy
    expect(dbl).to receive(:one).once.ordered

    dbl.one
    dbl.one
  end
end
  1)  should pass when it receives unexpected messages of itself
     Failure/Error: expect(dbl).to receive(:one).once.ordered
       (Double).one(*(any args))
           expected: 1 time with any arguments
           received: 2 times with any arguments
     # ./spec/order_spec.rb:13:in `block (2 levels) in <top (required)>'

  2)  should pass when it receives unexpected messages of itself
     Failure/Error: expect(dbl).to have_received(:one).once.ordered
       (Double).one(*(any args))
           expected: 1 time with any arguments
           received: 2 times with any arguments
     # ./spec/order_spec.rb:8:in `block (2 levels) in <top (required)>'

These both fail, and they both should fail -- they state the message should be received once and it was received twice.

RSpec.describe do
  # 3 FAILS
  it 'should pass when it receives messages of itself separated' do
    dbl = spy
    dbl.one
    dbl.between_one
    dbl.one

    expect(dbl).to have_received(:one).once.ordered
    expect(dbl).to have_received(:one).once.ordered
  end

  it 'should pass when it receives messages of itself separated' do
    dbl = spy

    expect(dbl).to receive(:one).once.ordered
    expect(dbl).to receive(:one).once.ordered

    dbl.one
    dbl.between_one
    dbl.one
  end
end

For this one, the have_received example fails but receive one passes:

Failures:

  1)  should pass when it receives messages of itself separated
     Failure/Error: expect(dbl).to have_received(:one).once.ordered
       (Double).one(*(any args))
           expected: 1 time with any arguments
           received: 2 times with any arguments
     # ./spec/order_spec.rb:9:in `block (2 levels) in <top (required)>'

RSpec.describe do
  # 4 FAILS
  it 'should pass when it receives messages of itself together' do
    dbl = spy
    dbl.one
    dbl.one

    expect(dbl).to have_received(:one).once.ordered
    expect(dbl).to have_received(:one).once.ordered
  end

  it 'should pass when it receives messages of itself together' do
    dbl = spy

    expect(dbl).to receive(:one).once.ordered
    expect(dbl).to receive(:one).once.ordered

    dbl.one
    dbl.one
  end
end

Same as the last one, the have_received example fails but receive one passes:

  should pass when it receives messages of itself together (FAILED - 1)
  should pass when it receives messages of itself together

Failures:

  1)  should pass when it receives messages of itself together
     Failure/Error: expect(dbl).to have_received(:one).once.ordered
       (Double).one(*(any args))
           expected: 1 time with any arguments
           received: 2 times with any arguments
     # ./spec/order_spec.rb:8:in `block (2 levels) in <top (required)>'

RSpec.describe do
  # 5 FAILS
  it 'should it pass when in order that matches expectation order,despite pre/post :ones ' do
    dbl = spy
    dbl.one
    dbl.two
    dbl.one
    dbl.three
    dbl.one

    expect(dbl).to have_received(:two).once.ordered
    expect(dbl).to have_received(:one).once.ordered
    expect(dbl).to have_received(:three).once.ordered
  end

  it 'should it pass when in order that matches expectation order,despite pre/post :ones ' do
    dbl = spy

    expect(dbl).to receive(:two).once.ordered
    expect(dbl).to receive(:one).once.ordered
    expect(dbl).to receive(:three).once.ordered

    dbl.one
    dbl.two
    dbl.one
    dbl.three
    dbl.one
  end
end

Both fail, and both should fail:

  should it pass when in order that matches expectation order,despite pre/post :ones (FAILED - 1)
  should it pass when in order that matches expectation order,despite pre/post :ones (FAILED - 2)

Failures:

  1)  should it pass when in order that matches expectation order,despite pre/post :ones
     Failure/Error: expect(dbl).to have_received(:one).once.ordered
       (Double).one(*(any args))
           expected: 1 time with any arguments
           received: 3 times with any arguments
     # ./spec/order_spec.rb:12:in `block (2 levels) in <top (required)>'

  2)  should it pass when in order that matches expectation order,despite pre/post :ones
     Failure/Error: dbl.one
       Double received :one out of order
     # ./spec/order_spec.rb:23:in `block (2 levels) in <top (required)>'

So ideally have_received would basically mirror the behavior of receive but I'm realizing that's not entirely possible. receive allows you to list all your expectations up front, before any messages are received, so you can list out the entire sequence. have_received, OTOH, checks each expectation as it was made because it is after-the-fact. As such, it's not able to take into account a later expectation.

I'm not sure where that leaves us. We never sad down to think about this in detail for have_received so there isn't any documented or planned behavior for these edge cases. Personally, I think it would be better to give the user a clear error (saying "have_received doesn't support this) than to support some of these edge cases but fail on others. I'm not yet sure what the logic should be for what we do and don't support, though....more thought is needed.

johnceh commented 9 years ago

Thanks for the input! I guess I got caught up in the original example having a spy vs double, when the real issue revolves closer to receive vs have_received. I can at least try to make the behavior match receive where we would expect (like in the examples above), and then maybe we'll see what edge cases are left and how to handle those.

ingobecker commented 8 years ago

I spent some time to figure out, that have_received is just working in a very limited way. I'm still unsure, if i totally understand in which cases it works and in which it doesn't. I think i would not have used have_received, if it would raise an error for every case that it could not handle. Also making things clear in the documentation would help. Especially the part about spies

have_received supports the same fluent interface for setting constraints that normal message expectations do.

in the documentation suggests a wong impression of the things that can be done withhave_received. I think i will convert all my tests to use receive because there are to many cases, in which it simply doesn't work as expected. I'm also unsure if it is a good idea to support have_received at all, if it is that complicated to understand or to explain in which cases it works and which it doesn't.

myronmarston commented 8 years ago

there are to many cases, in which [have_received] simply doesn't work as expected.

AFAIK, ordered is the only case which doesn't currently work as expected. Do you know of others?

ingobecker commented 8 years ago

I investigated a bit further. What confused me is that ordered works, as long as your function receives different arguments. For example

describe 'a double' do
  let(:dbl) { double('dbl') }
  before do
    allow(dbl).to receive(:foo)
  end

  it 'passes ordered with different arguments' do
    dbl.foo(0)
    dbl.foo(1)
    dbl.foo(2)

    expect(dbl).to have_received(:foo).with(0).ordered
    expect(dbl).to have_received(:foo).with(1).ordered
    expect(dbl).to have_received(:foo).with(2).ordered
  end
end

ordered works as expected. So it seems, the only case ordered doesn't work is, when you try to test for exactly the same messsage consecutively in one example:

describe 'a double' do
  let(:dbl) { double('dbl') }
  before do
    allow(dbl).to receive(:foo)
  end

  it 'fails ordered with same arguments consecutively' do
    dbl.foo(0)
    dbl.foo(1)
    dbl.foo(2)
    dbl.foo(0)

    expect(dbl).to have_received(:foo).with(0).ordered
    expect(dbl).to have_received(:foo).with(1).ordered
    expect(dbl).to have_received(:foo).with(2).ordered
    expect(dbl).to have_received(:foo).with(0).ordered
  end
end

That's the reason why i first thought ordered works in conjunction with have_received.

JonRowe commented 8 years ago

I think ordered isn't working at all in the first example, I suspect you'll find:

expect(dbl).to have_received(:foo).with(2).ordered
expect(dbl).to have_received(:foo).with(1).ordered
expect(dbl).to have_received(:foo).with(0).ordered

Also passes.

ingobecker commented 8 years ago
describe 'a double' do
  let(:dbl) { double('dbl') }
  before do
    allow(dbl).to receive(:foo)
  end

  it 'passes ordered with different arguments' do
    dbl.foo(0)
    dbl.foo(1)
    dbl.foo(2)

    expect(dbl).to have_received(:foo).with(2).ordered
    expect(dbl).to have_received(:foo).with(1).ordered
    expect(dbl).to have_received(:foo).with(0).ordered
  end
end

results in the following error:

a double
  passes ordered with different arguments (FAILED - 1)

Failures:

  1) a double passes ordered with different arguments
     Failure/Error: expect(dbl).to have_received(:foo).with(1).ordered
       #<Double "dbl"> received :foo out of order
     # /tmp/receive_ordered_spec.rb:13:in `block (2 levels) in <top (required)>'

Finished in 0.00994 seconds (files took 0.13633 seconds to load)
1 example, 1 failure

Failed examples:

rspec /tmp/receive_ordered_spec.rb:7 # a double passes ordered with different arguments
brunoald commented 5 years ago

I have the same problem as described by @ingobecker ... Has anyone figured out a fix for this?