Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Generate a useful description for have_received #256

Merged
merged 1 commit into from

3 participants

@jferris
  • Makes it { should have_received(:message) } produce useful output

Resolves #254.

A few implementation notes:

  • I added the actual description to ErrorGenerator, as it has a lot of useful, reusable behavior related to messages. However, it's not a message, so maybe it should go somewhere else, or that class should be renamed to "MessageGenerator"?
  • I separated Proxy#build_expectation into two phases so that exceptions won't be raised when calling matches? or description. Otherwise, you don't get a description if you mess up the stub.
features/spies/spy_pure_mock_method.feature
@@ -61,3 +61,16 @@ Feature: Spy on a stubbed method on a pure mock
When I run `rspec failed_message_expectations_spec.rb`
Then the output should contain "expected: (:expected, :arguments)"
And the output should contain "got: (:unexpected)"
+
+ Scenario: generate a spy message
+ Given a file named "spy_message_spec.rb" with:
+ """ruby
+ describe "have_received" do
+ subject { double('invitation', :deliver => true) }
+ before { subject.deliver }
@myronmarston Owner

Given the fact that we consider an explicit use of subject to be a smell (it's really just there to support the it { should } syntax), I think I'd prefer to see this use the named subject feature:

subject(:invitation) { double('invitation', :deliver => true) }
before { invitation.deliver }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff
lib/rspec/mocks/have_received.rb
@@ -31,6 +31,10 @@ def negative_failure_message
generate_failure_message
end
+ def description
+ expect.description
@myronmarston Owner

Is there any reason you are using expect.description rather than @expectation.description here? Seems like this will (unnecessarily) re-create the expectation.

@jferris
jferris added a note

If you call description before calling matches?, it will fail.

That's happened to me before when composing matchers from other matchers, but maybe I'm writing them wrong?

@myronmarston Owner

No, that makes sense...I had thought that rspec would only call #description after calling #matches?, but maybe that's not always the case. Or, even if that is the case, it's a bad idea to prevent other things from being able to call #description before #matches?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff
lib/rspec/mocks/proxy.rb
@@ -93,15 +104,11 @@ def build_expectation(method_name)
@error_generator.raise_expectation_on_unstubbed_method(method_name)
end
- expectation = meth_double.build_expectation(
- @error_generator,
- @expectation_ordering
- )
-
- yield expectation
@myronmarston Owner

It's not clear to me why this yield is going away -- can you explain this? (I want to make sure I understand what's going on here so I can support it well in the future).

@jferris
jferris added a note

I had the yield in there so that HaveReceived could mutate expectation before messages were replayed. I think this was kind of hard to follow, but couldn't figure out a way to tease it apart.

When adding description, calling expect was raising an exception, making it impossible to build the expectation when the stub was set up incorrectly. This pushed me towards splitting up build_expectation, so that it just builds the expectation and doesn't replay the messages. I made reply_received_message_on public so that the matcher can control which parts of the process it wants in which methods. Because the method is split up, it doesn't need to yield anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
spec/rspec/mocks/have_received_spec.rb
@@ -75,6 +75,11 @@ module Mocks
end
end
+ it 'generates a useful description' do
+ matcher = have_received(:expected_method).with(:expected_args).once
+ expect(matcher.description).to eq 'receive expected_method(:expected_args) 1 time'
+ end
+
@myronmarston Owner

I like the simplicity and conciseness of this spec :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
features/spies/spy_pure_mock_method.feature
@@ -61,3 +61,16 @@ Feature: Spy on a stubbed method on a pure mock
When I run `rspec failed_message_expectations_spec.rb`
Then the output should contain "expected: (:expected, :arguments)"
And the output should contain "got: (:unexpected)"
+
+ Scenario: generate a spy message
+ Given a file named "spy_message_spec.rb" with:
+ """ruby
+ describe "have_received" do
+ subject(:invitation) { double('invitation', :deliver => true) }
+ before { invitation.deliver }
+
+ it { should have_received(:deliver) }
+ end
+ """
+ When I run `rspec --format documentation spy_message_spec.rb`
+ Then the output should contain "should receive deliver"
@myronmarston Owner

It seems odd to me that the description is should receive deliver rather than should have received deliver given that the matcher is named have_received. Any reason you chose the former?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston myronmarston commented on the diff
features/spies/spy_pure_mock_method.feature
@@ -61,3 +61,16 @@ Feature: Spy on a stubbed method on a pure mock
When I run `rspec failed_message_expectations_spec.rb`
Then the output should contain "expected: (:expected, :arguments)"
And the output should contain "got: (:unexpected)"
+
+ Scenario: generate a spy message
+ Given a file named "spy_message_spec.rb" with:
+ """ruby
+ describe "have_received" do
+ subject(:invitation) { double('invitation', :deliver => true) }
@myronmarston Owner

The indentation here looks odd. (I can always take care of this post-merge -- just want to make sure we don't forget to fix it).

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

Looks great, @jferris. I just had one more question (about should receive vs should have received). I'll merge this soon.

@jferris

I didn't have a good reason for that, so I switched it to "have received." Agreed it makes more sense.

The build is currently failing on rbx-1.9-mode for this branch. I don't have that version installed locally and trying to build it failed for unclear reasons. I'll try rebuilding rbx to look at those failures as soon as I can.

@JonRowe
Owner

I think this needs rebasing before it can be merged, but we also need to fix those rbx failures.

@jferris jferris Generate a useful description for have_received
* Makes `it { should have_received(:message) }` produce useful output
1f6a7e3
@jferris

I rebased and force pushed to my branch today, and the rbx failures seem to be gone. That's a good thing, because I really can't seem to get Rubinius to compile.

@myronmarston myronmarston merged commit b5ddadc into rspec:master
@myronmarston

Thanks, @jferris!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 19, 2013
  1. @jferris

    Generate a useful description for have_received

    jferris authored
    * Makes `it { should have_received(:message) }` produce useful output
This page is out of date. Refresh to see the latest.
View
13 features/spies/spy_pure_mock_method.feature
@@ -61,3 +61,16 @@ Feature: Spy on a stubbed method on a pure mock
When I run `rspec failed_message_expectations_spec.rb`
Then the output should contain "expected: (:expected, :arguments)"
And the output should contain "got: (:unexpected)"
+
+ Scenario: generate a spy message
+ Given a file named "spy_message_spec.rb" with:
+ """ruby
+ describe "have_received" do
+ subject(:invitation) { double('invitation', :deliver => true) }
@myronmarston Owner

The indentation here looks odd. (I can always take care of this post-merge -- just want to make sure we don't forget to fix it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ before { invitation.deliver }
+
+ it { should have_received(:deliver) }
+ end
+ """
+ When I run `rspec --format documentation spy_message_spec.rb`
+ Then the output should contain "should have received deliver"
View
5 lib/rspec/mocks/error_generator.rb
@@ -74,6 +74,11 @@ def method_call_args_description(args)
end
# @private
+ def describe_expectation(message, expected_received_count, actual_received_count, *args)
+ "have received #{message}#{format_args(*args)} #{count_message(expected_received_count)}"
+ end
+
+ # @private
def raise_out_of_order_error(message)
__raise "#{intro} received :#{message} out of order"
end
View
19 lib/rspec/mocks/have_received.rb
@@ -13,14 +13,14 @@ def initialize(method_name)
def matches?(subject)
@subject = subject
@expectation = expect
- @expectation.expected_messages_received?
+ expected_messages_received?
end
def does_not_match?(subject)
@subject = subject
ensure_count_unconstrained
@expectation = expect.never
- @expectation.expected_messages_received?
+ expected_messages_received?
end
def failure_message
@@ -31,6 +31,10 @@ def negative_failure_message
generate_failure_message
end
+ def description
+ expect.description
@myronmarston Owner

Is there any reason you are using expect.description rather than @expectation.description here? Seems like this will (unnecessarily) re-create the expectation.

@jferris
jferris added a note

If you call description before calling matches?, it will fail.

That's happened to me before when composing matchers from other matchers, but maybe I'm writing them wrong?

@myronmarston Owner

No, that makes sense...I had thought that rspec would only call #description after calling #matches?, but maybe that's not always the case. Or, even if that is the case, it's a bad idea to prevent other things from being able to call #description before #matches?.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
CONSTRAINTS.each do |expectation|
define_method expectation do |*args|
@constraints << [expectation, *args]
@@ -41,9 +45,9 @@ def negative_failure_message
private
def expect
- build_expectation do |expectation|
- apply_constraints_to expectation
- end
+ expectation = mock_proxy.build_expectation(@method_name)
+ apply_constraints_to expectation
+ expectation
end
def apply_constraints_to(expectation)
@@ -72,8 +76,9 @@ def generate_failure_message
error.message
end
- def build_expectation(&block)
- mock_proxy.build_expectation(@method_name, &block)
+ def expected_messages_received?
+ mock_proxy.replay_received_message_on @expectation
+ @expectation.expected_messages_received?
end
def mock_proxy
View
5 lib/rspec/mocks/message_expectation.rb
@@ -257,6 +257,11 @@ def expectation_count_type
return nil
end
+ # @private
+ def description
+ @error_generator.describe_expectation(@message, @expected_received_count, @actual_received_count, *expected_args)
+ end
+
def raise_out_of_order_error
@error_generator.raise_out_of_order_error @message
end
View
34 lib/rspec/mocks/proxy.rb
@@ -85,6 +85,17 @@ def add_negative_message_expectation(location, method_name, &implementation)
def build_expectation(method_name)
meth_double = method_double[method_name]
+ meth_double.build_expectation(
+ @error_generator,
+ @expectation_ordering
+ )
+ end
+
+ # @private
+ def replay_received_message_on(expectation)
+ method_name = expectation.message
+ meth_double = method_double[method_name]
+
if meth_double.expectations.any?
@error_generator.raise_expectation_on_mocked_method(method_name)
end
@@ -93,15 +104,11 @@ def build_expectation(method_name)
@error_generator.raise_expectation_on_unstubbed_method(method_name)
end
- expectation = meth_double.build_expectation(
- @error_generator,
- @expectation_ordering
- )
-
- yield expectation
@myronmarston Owner

It's not clear to me why this yield is going away -- can you explain this? (I want to make sure I understand what's going on here so I can support it well in the future).

@jferris
jferris added a note

I had the yield in there so that HaveReceived could mutate expectation before messages were replayed. I think this was kind of hard to follow, but couldn't figure out a way to tease it apart.

When adding description, calling expect was raising an exception, making it impossible to build the expectation when the stub was set up incorrectly. This pushed me towards splitting up build_expectation, so that it just builds the expectation and doesn't replay the messages. I made reply_received_message_on public so that the matcher can control which parts of the process it wants in which methods. Because the method is split up, it doesn't need to yield anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
-
- replay_received_message_on expectation
- expectation
+ @messages_received.each do |(method_name, args, _)|
+ if expectation.matches?(method_name, *args)
+ expectation.invoke(nil)
+ end
+ end
end
# @private
@@ -234,15 +241,6 @@ def find_matching_method_stub(method_name, *args)
def find_almost_matching_stub(method_name, *args)
method_double[method_name].stubs.find {|stub| stub.matches_name_but_not_args(method_name, *args)}
end
-
- def replay_received_message_on(expectation)
- @messages_received.each do |(method_name, args, _)|
- if expectation.matches?(method_name, *args)
- expectation.invoke(nil)
- end
- end
- end
-
end
end
end
View
5 spec/rspec/mocks/have_received_spec.rb
@@ -75,6 +75,11 @@ module Mocks
end
end
+ it 'generates a useful description' do
+ matcher = have_received(:expected_method).with(:expected_args).once
+ expect(matcher.description).to eq 'have received expected_method(:expected_args) 1 time'
+ end
+
context "counts" do
let(:dbl) { double(:expected_method => nil) }
Something went wrong with that request. Please try again.