rspec / rspec-mocks

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

`allow(...).to_not receive` should exist #872

Closed krainboltgreene closed 9 years ago

krainboltgreene commented 9 years ago

allow(...).to_not receive is not supported since it doesn't really make sense. What would it even mean?

It would mean disabling/deleting a stub if it exists, the opposite of #to.

If I set a allow in an before, where it's used in 90% of nested tests but want to disable a stub for a single test inside of that, this would be what I would reach for.

myronmarston commented 9 years ago

It would mean disabling/deleting a stub if it exists, the opposite of #to.

But the English expression "allow [noun] to not [verb]" is pretty nonsensical. As a parent, I allow my children to go in our backyard and play outside. When I revoke that privilege, I would never say "I allow them to not go outside". "Allow" simply isn't the right verb for that situation. I'm open to adding an API to do what you want (the equivalent of unstub in the old monkey-patched syntax) if we come up with a good, non-monkey patching API for it that fits in with allow(...).to receive and expect(...).to receive...but we haven't been able to come up with it yet. We discussed it at length in #153 and couldn't come up with something we were happy with. For the common case of a partial double that you want to restore a method to what it originally did, you can always do:

allow(object).to receive(:message).and_call_original

...which is (almost) the same as unstub was -- the only difference is that we still observe calls to object.message.

krainboltgreene commented 9 years ago
allow(object).to receive(:message).and_call_original

Does that also undefine the method?

Regardless of the english sensicality, it's the exact opposite of it's counterpart and fits with the semantics. I'm writing code, not an article.

myronmarston commented 9 years ago

Does that also undefine the method?

No; the method is still defined (so that RSpec can observe it -- necessary for the have_received matcher), but it will act like it originally did. For example in this example:

dbl = Object.new
allow(dbl).to receive(:foo).and_return(2)
allow(dbl).to receive(:foo).and_call_original

dbl.foo

...the last line (dbl.foo) raises this error:

     NoMethodError:
       undefined method `foo' for #<Object:0x007fe6fd141018>

...which is what dbl.foo originally did before we stubbed it. You'll notice that there are extra rspec-mocks frames in the stacktrace, though, because we still observe it, like I said.

Regardless of the english sensicality, it's the exact opposite of it's counterpart and fits with the semantics. I'm writing code, not an article.

Regardless of your specific needs, one of RSpec's goals has always been to have APIs that read, in English, exactly like what they do, and this does not fit in with that goal.

If you want the method to be undefined you might be able to use Ruby's undef_method and/or remove_method. You might also consider using metadata to prevent the before hook that stubs the method from applying to the specific examples where you don't want it. Example:

RSpec.describe do
  before do |ex|
    allow(Object).to receive(:foo).and_return(2) unless ex.metadata[:skip_object_stub]
  end

  it 'is stubbed for this example' do
    expect(Object.foo).to eq(2)
  end

  it 'is not stubbed for this example', :skip_object_stub do
    expect { Object.foo }.to raise_error(NoMethodError)
  end
end

This is going to perform better than stubbing and unstubbing, anyway...it's always faster to not do something than it is to do it and then undo it.

jasonkarns commented 9 years ago

Missing this feature.

The code under test relies on respond_to, which we're attempting to test. All but one of the tests need the method stubbed, so we have allow().to receive in before. And we need to remove that method stub for a single test.

undef_method and remove_method don't work because rspec complains about the double receiving unexpected messages :undef_method and :remove_method

An alternative is to push all the tests into a context that require the method to be stubbed. Thus leaving the single negation test separate, with a unstubbed-double. However, this is not appealing because the test is not explicit about the method being undefined. It is implicit that the method is undefined simply by virtue of it not being stubbed. explicit > implicit

There don't seem to be any good options without this feature. For now, I'm going to continue to use unstub from the :should syntax and just ignore rspec's warning message.

myronmarston commented 9 years ago

@jasonkarns -- can you put together an example of what you are trying to do?

jasonkarns commented 9 years ago

This is what we currently have:


class Subject
  attr_accessor :page

  def foo
    page.tap {|f| raise Error unless f.respond_to?(:execute_script) }
  end
end

describe "#foo" do
  let(:page) { double('page') }

  before :each do
    allow(page).to receive(:execute_script)
    subject.page = page
  end

  # bunch of specs around other logic in #foo
  # ...

  it "should ensure the page is webdriver-esque" do
    page.unstub(:execute_script)
    expect { subject.page("foo") }.to raise_error
  end
end
myronmarston commented 9 years ago

Thanks, that's helpful.

The reason undef_method and remove_method did not work for you is that they are defined on Module, not Object, and RSpec's doubles are not modules. To use them on a double, you just need to run them in the context of the singleton class. Here's an example that works:

RSpec.describe "Unstubbing with `undef`" do
  it 'makes the double no longer respond to the method' do
    dbl = double(foo: "a")
    expect {
      class << dbl
        undef foo
      end
    }.to change { dbl.respond_to?(:foo) }.from(true).to(false)
  end
end
myronmarston commented 9 years ago

BTW, I'm open to adding an equivalent of unstub for the newer syntax if someone can come up with an API that makes sense and reads well.

jasonkarns commented 9 years ago

I recognize the grammar of allow(x).not_to receive is quite odd. But it fits with the rest of rspec's api which makes it intuitive (IMO). But I know you've add a full discussion on that, so I don't want to re-start it. :)

jasonkarns commented 9 years ago

Mostly, I just wanted to add my vote that this feature is still desired, and I (as a user, not maintainer, so grain of salt) would be willing to live with a less grammatically correct API as long as the feature is available.

disallow(x).to receive would also work, but that's a bigger API change (and could open a can of worms with the with/return constraints)

jasonkarns commented 9 years ago

going with class << page; undef execute_script end for now since it's explicit about the method being undefined, and is relatively clear enough (compared to page.unstub(:execute_script)) that it's worth using to eliminate the unstub deprecation warning. Thanks @myronmarston

maxshortzp commented 7 years ago

Another potential workaround: appending and_raise e.g. and_raise('Expected this method to not be called.')

rpbaptist commented 6 years ago

I came here, because I accidentally put allow().not_to receive and was surprised by the error message. Beyond stating that this is not supported I find the message unhelpful and unnecessarily condescending.

JonRowe commented 6 years ago

PR welcome if you'd like to improve the clarity, the original intent was humorous, as it the allow().not_to receive is a no operation / tautology

vchervanev commented 1 year ago

My workaround from 2023 is

allow(Klass).to receive(:method).at_most(0).times

(need it because we warn about using expects in befores and it makes sense in general but should be whitelisted to "disallow" receiving a message)