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

Already on GitHub? Sign in to your account

.with() has no effect on .and_yield() #127

Closed
billbillington opened this Issue Apr 19, 2012 · 2 comments

Comments

Projects
None yet
2 participants

I found that that I couldn't get rspec to recognise two calls to the same method If one takes a block and the other dosen't.

This can cause problems when testing methods that produces some default output that can (optionally) wrap content passed in s block (e.g. the content_tag html helpers)

I have written a failing spec illustrating the problem

require 'spec_helper'

class ExpectationFail
  def perform(obj1, obj2)
    optional_yielder(obj2) do
      optional_yielder(obj1)
    end
  end

  def optional_yielder(obj, &block)
    block_given? ? "before yield #{yield} after yield"  : '[value]'
  end
end

describe ExpectationFail do
  let(:obj1) { double 'obj1' }
  let(:obj2) { double 'obj2' }

  before(:each) do
    subject.stub(:optional_yielder).with(obj2).and_yield
  end

  specify "perform should make call to 'inner' optional_yeilder" do
    subject.should_receive(:optional_yielder).with(obj1)
    subject.perform(obj1, obj2)
  end
end

Looking at the source, I think that the problem is that @args_to_yield in MessageExpectation, by design, doesn't have any concept of expectations and as sucht applies globally to all calls of a method.

I'm not sure if this can be considered a bug but I thought I'd bring it up as I found this behaviour to be quite surprising as the 'and' in and_yeild() suggests to me that the methods behaviour is affected by the code before it in a simular way to and_return()

Owner

myronmarston commented Dec 26, 2012

@billbillington -- thanks for reporting this, and sorry for it taking so long for any of us to get back to you!

We try to respond to open issues promptly but working on rspec is a volunteer effort and sometimes we miss tickets. Feel free to post follow up comments to remind us of your issues so we don't let this happen again.

Anyhow, I pushed a failing spec into a branch. I've dug into this a fair bit and I haven't found an easy fix. Actually, I've traced this to a deeper problem with the way rspec-mocks works, as this failing example (which dosen't involve and_yield at all) demonstrates:

    it 'uses the correct stubbed response when responding to a mock expectation' do
      obj.stub(:foo) { 15 }
      obj.stub(:foo).with(:eighteen) { 18 }

      obj.should_receive(:foo)
      expect(obj.foo(:blah)).to eq(15)
    end

This fails, because obj.foo(:blah) returns 18, not 15 as expected. The basic problem is here:

      def add_expectation(error_generator, expectation_ordering, expected_from, opts, &implementation)
        configure_method
        expectation = if existing_stub = stubs.first
                        existing_stub.build_child(expected_from, 1, opts, &implementation)
                      else
                        MessageExpectation.new(error_generator, expectation_ordering,
                                               expected_from, self, 1, opts, &implementation)
                      end
        expectations << expectation
        expectation
      end

When adding an expectation, it is built as a child of an existing stub. The problem is that it indiscriminately picks the first stub (which is actually the last declared stub due to the use of unshift), even though the received message args may not match the argument constraints on that stub. When the child is built it even sets the argument list matcher to any_args. Thus, when the message is received and the expectation invoked, it uses the implementation of a stub that wasn't designed to match the given message (since the args don't match). In @billbillington's original example it used the and_yield stub even though the args didn't match; in my example above, it used the 18 stub even though the args didn't match.

I'm not sure what we should do to fix this yet. Building a child off of the first stub seem suspect. But I don't think we can "smartly" pick the correct stub at this point, either -- we don't know what the received args will be. I think we may need to rewrite things here so that the message expectation lazily selects its parent when the message is received based on which stub matches the given args...but I'm not sure how feasible that is. I've never really worked with the build_child stuff in rspec-mocks so I'm not easily going to be able to fix this.

@dchelimsky -- any thoughts on how we can fix this? It seems like a pretty deep flaw in the build_child stuff but I have no idea what to do instead.

Owner

myronmarston commented Dec 26, 2012

@alindeman -- Forgot to cc you on this above...but I'd love to get your thoughts on this as well.

@ghost ghost assigned myronmarston Jan 31, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment