Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Issue #123 #147

Merged
merged 1 commit into from

3 participants

@Bodhisattva2-0

Improved error message when user forgets to stub a method with default behavior. Based on this definition of 'with'.

"Constrains a stub or message expectation to invocations with specific arguments. With a stub, if the message might be received with other args as well, you should stub a default value first, and then stub or mock the same message using with to constrain to specific arguments."

lib/rspec/mocks/error_generator.rb
@@ -28,6 +28,13 @@ def raise_unexpected_message_args_error(expectation, *args)
end
# @private
+ def raise_no_stub_with_default_behavior(expectation,*args)
@dchelimsky Owner

How about raise_no_default_stub_error or raise_missing_default_stub_error? I think either aligns better with the other raise_xxx_error method names. WDYT?

'raise_missing_default_stub_error' sounds better of the lot. I'll make the changes. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/rspec/mocks/proxy.rb
@@ -129,6 +129,8 @@ def message_received(message, *args, &block)
elsif stub = find_almost_matching_stub(message, *args)
stub.advise(*args)
raise_unexpected_message_args_error(stub, *args)
+ elsif expectation = find_almost_matching_satisfied_expectation(message,*args)
+ @error_generator.raise_no_stub_with_default_behavior(expectation, *args) if not null_object?
@dchelimsky Owner

This should be wrapped in a local private method to align with the other branches that raise.

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

Made both suggested changes.

@dchelimsky dchelimsky commented on the diff
spec/rspec/mocks/bug_report_123_spec.rb
@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe 'with' do
+ it "should ask the user to stub a default value first if the message might be received with other args as well" do
+ obj = Object.new
+ obj.should_receive(:foobar).with(1)
@dchelimsky Owner

I merged locally after you made my suggested changes. All the specs pass but then when I tried it it didn't actually work as expected yet. If you change this example to use stub instead of should_receive, you'll see it fail.

The reason is that find_almost_matching_stub returns the stub and we never get to the next conditional. I'm thinking we can just delegate this branch to raise_missing_default_stub_error instead of raise_unexpected_message_args_error and that solves the problem. WDYT?

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

I'm going to go ahead and take what you've got and go from there. Thanks for the pull!

@dchelimsky dchelimsky merged commit 1ebc698 into rspec:master
@dchelimsky
Owner

I made some adjustments in 49ce496.

@travisbot

This pull request passes (merged 409955f into 4b70b45).

@Bodhisattva2-0 Bodhisattva2-0 referenced this pull request from a commit in c42engineering/rspec-mocks
@dchelimsky dchelimsky Use the improved message from the prev commit for stubs, not message …
…expectations.

- Closes #147, #123.
49ce496
@alindeman alindeman referenced this pull request
Closed

#with and stubbing #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 4, 2012
  1. Issue #123: Improved error message when user forgets to stub a method…

    Neha Kumari authored
    … with default behavior.
This page is out of date. Refresh to see the latest.
View
7 lib/rspec/mocks/error_generator.rb
@@ -28,6 +28,13 @@ def raise_unexpected_message_args_error(expectation, *args)
end
# @private
+ def raise_missing_default_stub_error(expectation,*args)
+ expected_args = format_args(*expectation.expected_args)
+ actual_args = args.collect {|a| format_args(*a)}.join(", ")
+ __raise "#{intro} received #{expectation.message.inspect} with unexpected arguments\n Please stub a default value first if message might be received with other args as well. \n"
+ end
+
+ # @private
def raise_similar_message_args_error(expectation, *args)
expected_args = format_args(*expectation.expected_args)
actual_args = args.collect {|a| format_args(*a)}.join(", ")
View
11 lib/rspec/mocks/proxy.rb
@@ -129,6 +129,8 @@ def message_received(message, *args, &block)
elsif stub = find_almost_matching_stub(message, *args)
stub.advise(*args)
raise_unexpected_message_args_error(stub, *args)
+ elsif expectation = find_almost_matching_satisfied_expectation(message,*args)
+ raise_missing_default_stub_error(expectation, *args) if not null_object?
elsif @object.is_a?(Class)
@object.superclass.__send__(message, *args, &block)
else
@@ -146,6 +148,11 @@ def raise_unexpected_message_error(method_name, *args)
@error_generator.raise_unexpected_message_error method_name, *args
end
+ # @private
+ def raise_missing_default_stub_error(expectation, *args)
+ @error_generator.raise_missing_default_stub_error(expectation, *args)
+ end
+
private
def method_double
@@ -165,6 +172,10 @@ def find_almost_matching_expectation(method_name, *args)
method_double[method_name].expectations.find {|expectation| expectation.matches_name_but_not_args(method_name, *args) && !expectation.called_max_times?}
end
+ def find_almost_matching_satisfied_expectation(method_name, *args)
+ method_double[method_name].expectations.find {|expectation| expectation.matches_name_but_not_args(method_name, *args) }
+ end
+
def find_matching_method_stub(method_name, *args)
method_double[method_name].stubs.find {|stub| stub.matches?(method_name, *args)}
end
View
10 spec/rspec/mocks/bug_report_123_spec.rb
@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe 'with' do
+ it "should ask the user to stub a default value first if the message might be received with other args as well" do
+ obj = Object.new
+ obj.should_receive(:foobar).with(1)
@dchelimsky Owner

I merged locally after you made my suggested changes. All the specs pass but then when I tried it it didn't actually work as expected yet. If you change this example to use stub instead of should_receive, you'll see it fail.

The reason is that find_almost_matching_stub returns the stub and we never get to the next conditional. I'm thinking we can just delegate this branch to raise_missing_default_stub_error instead of raise_unexpected_message_args_error and that solves the problem. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ obj.foobar(1)
+ lambda{ obj.foobar(2) }.should raise_error(RSpec::Mocks::MockExpectationError, /Please stub a default value first if message might be received with other args as well./)
+ end
+end
Something went wrong with that request. Please try again.