File.stub(:exist?).with(x) causes NoMethodError when File.exist?(y) is called

[myronmarston]

I'm specing some behavior that depends on the existence or absence of a file. I'm doing so with a stub:

context 'when the file exists' do
  before(:each) { File.stub(:exist?).with(file_name).and_return(true) }

  it "returns :bar from #foo" do
    described_class.new(file_name).foo.should == :bar
  end
end

This works fine except that described_class's initializer also calls File.exist? with a different argument. That causes an error: NoMethodError: undefined method 'exist?' for IO:Class.

It seems to me that an argument-limited stub of an existing method should delegate to the original method definition when the method is called with a different argument, rather than raising a NoMethodError. My stubbing of File.exist? for file x should not effect File.exist?(y).

[dchelimsky]

My instinct is that this would be a big enough change in current behaviour as to be not only disruptive, but potentially harmful to anyone who's specs are stubbing a method that does something destructive.

Agreed that the error message is misleading and can be improved.

Agreed that we should have a way to delegate to the real method, but I'd want it to be explicit.

WDYT?

[myronmarston]

I hadn't considered the case where you're stubbing something destructive. Now that you bring it up, I agree that object.stub(:something_descructive).with(x) should guarantee that the destructive thing never happens, regardless of the argument.

I like the idea of having an explicit way to allow delegation to the real method when other arguments are passed, like you suggest. Would it just be an additional method call on the fluent interface? Something like this, perhaps:

File.stub(:exist?).with(x).and_return(true).and_delegate_to_original_method_definition_otherwise

...but and_delegate_to_original_method_definition_otherwise is a pretty terrible name for this. I can't think of a better name, though :(.

And I also agree that a better error message than NoMethodError would be really helpful when you don't configure it to delegate. Preferably, the error message would go hand-in-hand with the delegation configuration option, and provide guidance of how to set that up.

So yes, I agree with your conclusions :). I'm glad I didn't go ahead and start working on this...

[myronmarston]

Actually, thinking about this some more, I think that you should not be limiting the stub arguments by calling with if you're stubbing something destructive and you want a guarantee that the destructive thing never occurs. Maybe this is just how my brain thinks about stubbing, but I think of with as limiting the scope of the stub to only applying when the arguments match, and leaving the existing behavior intact otherwise.

Obviously, others may not expect it to work this way, especially given how rspec-mocks work currently. I'd prefer the API work how I've suggested, but given the past behavior and the potential for breakage, I'm fine with doing what you've suggested. Still, it's nice not to be stuck with a sub-optimal API due to expectations of how things worked previously, and if you're going to break an API, a major release (i.e. RSpec 2) is the time to do it.

[dchelimsky]

Tell you what - why don't you write to the rspec-users list and run this by a wider audience. Ask people to comment here rather than on the list so we can have the discussion all in one place.

[phillipkoebbe]

I can't remember the details, but I recall stumbling upon this situation in some form. I have a couple of projects each with over 1500 specs, so it will take me a bit to figure out where I encountered this before I could contribute much to the conversation. But I will say this: if I'm stubbing, I never want the real thing to be called without me making an explicit step to do so. If I have something stubbed as .with(x) to have the real method called if y is passed would violate the principle of least surprise. I'd be quite surprised! I doubt that it's very good idea to begin with, but I've wondered if it would be possible for stubbing to keep track of all the different permutations that we might want to define, such as:

my_object.stub(my_method)
my_object.stub(my_method).with(x)
my_object.stub(my_method).with(y).and_return(z)

Could all of those be mapped somehow? I can't remember what it was, but there was a time when I wished I could do that. It might have been a terrible spec on my part, though. I can't recall.

[dchelimsky]

@phillipkoebbe - that actually works now as/is.

[myronmarston]

If I have something stubbed as .with(x) to have the real method called if y is passed would violate the principle of least surprise. I'd be quite surprised!

Funny you mention that--I was surprised that it didn't delegate to the original method definition when y was passed. object.stub(my_method).with(x) appears to me to be completely silent about what to do for object.my_method(y)--so I expected it to not change its behavior.

[phillipkoebbe]

@dchelimsky

My bad. I didn't realize we were talking about v2. I haven't made the move to Rails 3 / RSpec 2 yet, so my mind is still thinking in v1. Is the behavior I described in v1 or am I just imagining things?

@myronmarston

Heh heh. I guess we aren't all surprised by the same things. I suppose what I was expecting was if something is stubbed, it's stubbed, regardless of what is passed to it. But now that I think about it more, considering your comment and my own previous thought about the various permutations, I can see how your expectation actually makes more sense. The stubbing framework can do only what I tell it to do, so if I don't tell it what to do with y, then it probably shouldn't do anything. In my reconsidered opinion, then, my preference would be to delegate to the original only if

my_object.stub(my_method)

is not defined. Since this is a blanket stub irrespective of arguments, it should be considered a "catch all" for stubbing. If that is not defined, then any argument passed that is not explicitly stubbed should be delegated to the original. That, at the moment, makes sense to me.

[txus]

What if you tell the stub something like:

my_object.stub(:method).exclusively_with(:some_dangerous_args)  # or
my_object.stub(:method).explicitly_with(:some_dangerous_args)

Would that be clear enough to assume delegation to the original with other arguments than those stated by exclusively_with / explicitly_with ?

Just that it is a bit less clear when stubbing this way in two separate statements (assuming exclusively_with for example):

my_object.stub(:method).exclusively_with(:some_dangerous_args)
... code ...
my_object.stub(:method).exclusively_with(:some_other_dangerous_args)

Seems that the 'exclusivity' of the second statement prevails, which (I think) would not be the desired behavior. Any thoughts on this?

[myronmarston]

@txus--interesting idea. I'm not sure the name is right, though--I had to read it several times before I figured out what it would do. Overall, I'd still prefer we use with and delegate any calls with arguments that don't match any of the stub argument matchers.

David: you brought up the issue of existing specs that stub a dangerous method, and that my suggested change could now allow the dangerous method to be called. Given the current behavior, I doubt there are many (if any) cases of this in the wild. Consider this code:

describe SomeClass do
  before(:each) do
    SomeClass.stub(:dangerous_method).with("foo")
  end

  it "works with foo" do
    SomeClass.dangerous_method("foo")
  end

  it "works with bar" do
    SomeClass.dangerous_method("bar")
  end
end

Your concern, as I understand it, is that the 2nd spec would now do the dangerous thing. And this is correct. But the 2nd spec won't pass with rspec-mock's current behavior, as a NoMethodError would be raised. So I doubt you'd see this spec in the wild.

[dchelimsky]

@myronmarston - that may be so, but it doesn't protect against refactorings that change the implementation such that it exposes the call to the dangerous method.

What about something like this:

obj = SomeClass.new
obj.stub(:dangerous_method) do |arg|
  arg == "foo" ? "bar" : call_original
end

[txus]

+1 for this one, call_original is definitely explicit.

[myronmarston]

I like this syntax quite a bit, too.

That said, this is definitely more verbose than obj.stub(:dangerous_method).with('foo'), and even though I'd like my stubs to always call the original when not matching the specified args, I doubt I'll use this regularly simply because of the verbosity. Hopefully, an appropriate, clear error will be raised for obj.dangerous_method('bar') so that I'm informed that the invocation doesn't match my stub, and I can switch it to use call original.

[dchelimsky]

Raise with unexpected args message instead of NoMethodError when a stub
is specified with args but received with different args: 409b2d0

I'm going to close this one and open a new one to add call_original