Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple message allowances/expectations via `receive_messages` #399

Merged
merged 28 commits into from Sep 12, 2013

Conversation

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Aug 9, 2013

Further to #368 this allow multiple message allowances/expectations via receive_messages.

receiver.send(method_name,subject,&block)
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

It seems like a lot of overhead to go through to repeatedly delegate to the Receive matcher here (which in turn delegates to a more fundamental underlying mechanism). Among other things, if you have provided a hash of 6 messages, the receive matchers will each get the mock proxy individually, causing 6 mock proxy lookups when one would suffice.

What would it look like to bypass the receive matcher?

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

It's the same as setting up each expectation / allowance, so it's no additional overhead, but it looked to me like it would involve a fair amount of repetition to extract this out...

This comment has been minimized.

@myronmarston

myronmarston Aug 10, 2013
Member

Actually, in #401 @xaviershay has some perf improvements that this PR should take advantage of, I think: he's introduced the concept of a "simple stub" for these sorts of message/return-value pairs, and it looks to greatly improve perf.

So, I'm thinking we should merge his PR first (it sounds like he'll address my feedback soon and it's very close to being ready to merge) and then refactor this to use add_simple_stub under the covers.

Thoughts?

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I like that idea, I'll take a look when it's merged.

].each do |method_name|
define_method(method_name) do |subject, &block|
@receivers.each do |receiver|
receiver.send(method_name,subject,&block)

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

If we do stick with this implementation, please put spaces, between, your, arguments.

@@ -11,7 +11,7 @@ def initialize(target)
def self.delegate_to(matcher_method, options = {})
method_name = options.fetch(:from) { :to }
define_method(method_name) do |matcher, &block|
unless Matchers::Receive === matcher
unless Matchers::Receive === matcher || Matchers::ReceivesMessages === matcher
raise UnsupportedMatcherError, "only the `receive` matcher is supported " +

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

The message should probably change to mention receive and receive_messages.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

Updated the message.

module RSpec
module Mocks
describe "allow(...).to receive_messages(:a => 1, :b => 2)" do
let(:obj) { double "Object" }

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

Why bother with the "Object" argument here?

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I like having a name, I can drop it if needs be.

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

I think having a name is useful when your test double is playing a role. The role it's playing here is "generic test double"...which really isn't a named role.

Not a big deal either way, though, I was just curious :).

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

TBH It's mostly force of habit from having multiple doubles, which when anonymous makes it hard to see which is which.

it "complains if a block is given" do
expect do
allow(obj).to receive_messages(:a => 1) { "implementation" }
end.to raise_error "Implementation blocks arn't supported with `receive_messages`"

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

This isn't a big deal at all (and I'm sure we have other places that do what you've done here), but I prefer curly braces for block expectations over do...end because the do/end words obscure the English phrasing. Thoughts on using curlies instead?

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I prefer do...end for multi-line blocks... that's all, I'm ok with changing these...

This comment has been minimized.

@myronmarston

myronmarston Aug 10, 2013
Member

I generally do, too...but I make an exception when the expression is meant to read like an English phrase, since "do" and "end" are english words. That's just my preference, though. And as I said, it's not a big deal, and certainly not a merge blocker.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I switched em anyway.


it "sets up multiple expectations" do
expect(reporter).to receive(:example_passed).with("will pass")
expect(reporter).to receive(:example_failed).with("will fail")

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

Mocking the reporter seems kinda like an odd and potentially brittle way to demonstrate pass/fail...

obj.a
end
end
example_group.run reporter

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

I'm a fan of creating example groups and running them from within a spec like this for specs in rspec-core, but it has a bunch of sandboxing to assist. We don't have that here. In addition, it seems kinda complicated/unnecessary. Elsewhere we use an expression like:

expect { verify obj }.to raise_error(RSpec::Mocks::MockExpectationError)

...or:

expect { verify obj }.not_to raise_error

You can also use verify_all in place of verify obj. For an example of this, see receive_spec.rb.

If you switch to this way of testing it you don't need to mess with mocking the reporter.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I've refactored these, take a look?

end
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

I don't see anything here for allow_any_instance_of or expect_any_instance_of. Those should be supported as well, right?

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

Added

raise "Implementation blocks arn't supported with `receive_messages`" if block_given?
Matchers::ReceivesMessages.new(method_value_hash)
end

This comment has been minimized.

@myronmarston

myronmarston Aug 9, 2013
Member

Would be good to add yard docs for this method. Due to the way these methods get defined at runtime, yard doesn't pick them up if we put the docs directly above them. Instead, see the bottom of this file -- the docs are there.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

Yup, I'll make sure to do that.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

Done.

@@ -321,6 +327,18 @@ def self.default_should_syntax_host
# expect(obj).to receive(:hello).with("world").exactly(3).times
#
# @note This is only available when you have enabled the `expect` syntax.
#
# @method receive_messages
# Used to specify multiple messages that you expect (or allow) an

This comment has been minimized.

@myronmarston

myronmarston Aug 10, 2013
Member

This implies it's only for the case when you have multiple messages. It's also for the case when you have a single message/return value pair and want the convenience of the hash syntax.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

Refactored, WDYT now?

# @method receive_messages
# Used to specify multiple messages that you expect (or allow) an
# object to receive. The method takes a hash of method names and
# their respective return values. Unlike `receive` block implementations

This comment has been minimized.

@myronmarston

myronmarston Aug 10, 2013
Member

Rather than saying a "hash of method names and their respective return values", can we say a "hash of message/return-value pairs"? We use the term "message" pretty consistently rather than "method name" and it's more consistent with the "receive" phrasing we've chosen to use.

This comment has been minimized.

@JonRowe

JonRowe Aug 10, 2013
Author Member

I was looking for message name, which didn't fit, so went to method name, reverted to message.

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Aug 12, 2013

As promised, refactored to use the improvements from #401

@@ -28,6 +28,7 @@
require 'rspec/mocks/mutate_const'
require 'rspec/mocks/matchers/have_received'
require 'rspec/mocks/matchers/receive'
require 'rspec/mocks/matchers/receives_messages'

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

The matcher is receive_messages not receives_messages....shouldn't the file be named accordingly?

@@ -92,6 +92,11 @@ def receive(method_name, &block)
Matchers::Receive.new(method_name, block)
end

def receive_messages(method_value_hash)
raise "Implementation blocks arn't supported with `receive_messages`" if block_given?

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

s/arn't/aren't/

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

Good catch

@@ -92,6 +92,11 @@ def receive(method_name, &block)
Matchers::Receive.new(method_name, block)
end

def receive_messages(method_value_hash)

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

For consistency, I think it'd be good to name this message_return_value_pairs to align with receive_messages. Thoughts?

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

Agree on message_return_value but sticking with hash over pairs as I feel it makes it clearer what the argument is expected to be.

# @method receive_messages
# Shorthand syntax used to setup message(s), and their return value(s),
# that you expect or allow an object to receive. The method takes a hash
# of messages and their respective return values. Unlike `receive` block

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

I think that this:

Unlike `recieve` block

Should be:

Unlike `receive`, block

(The comma seems important for it to read correctly...)

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

Yep, agree

alias matches? setup_expectation
alias does_not_match? setup_expectation
alias setup_negative_expectation setup_expectation
alias setup_allowance setup_expectation

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

Why are all these aliased to setup_expectation?

  • setup_allowance is overriden below.
  • The negative ones (does_not_match and setup_negative_expectation) seem wrong to be the same thing as the positive case.

Actually, I'm not sure this should support the negative case at all...what would this mean?

expect(object).not_to receive_messages(foo: 3, bar: 25)

It's confusing since return values don't make sense for the negative case. Would be good to spec this, of course.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

They were supposed to be the same, but I misunderstood how the negation was being applied, I've dropped the negative case since I don't think it makes sense (and I don't believe it's useful to have such negative assertions anyway, I prefer just not stubbing the methods)

@message_value_hash.each do |message, value|
host.__send__(method_name, message, *args).and_return(value)
end
end

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

I find the names map_to and map_to_as_chain to be confusing...it's not clear to me what they are supposed to mean. Can you explain them? Or maybe a different name is better...I'm unsure right now.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

I've refactored this to make it clearer

@@ -48,6 +48,11 @@ def raise_expectation_error(message, expected_received_count, argument_list_matc
end

# @private
def raise_simple_expectation_error(message)
__raise "(#{intro}).#{message} expected with any arguments"

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

Is this the same message phrasing as has been returned for a normal expectation?

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

Actually, is there even a need for a different method/message here?

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

The more complex error generator method has more things that the SimpleMessageExpectation doesn't know about, it seemed cleaner to be able to describe a simpler message, the language could use some tweaks though.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

Ok, I figured out the proper error generator :), so this is gone.

module RSpec
module Mocks
module Matchers
class ReceivesMessages

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

I think the class should be ReceiveMessages since the matcher method is receive_messages.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

I though it read better, but I'm not overly attached to it, changed.

raise UnsupportedMatcherError, "only the `receive` matcher is supported " +
unless Matchers::Receive === matcher || Matchers::ReceivesMessages === matcher
raise UnsupportedMatcherError,
"only the `receive` or `receive_messages` matchers are supported " +

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

I think you want "and" not "or" here....after all, both matchers are supported.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

I prefer "or" as either matcher is supported... maybe it should read "and/or"...

@message_return_value_hash.each do |message, value|
yield host, message, value
end
end

This comment has been minimized.

@myronmarston

myronmarston Aug 13, 2013
Member

This is much more readable/understandable than the map_on thing you had before. Thanks!

One suggestion, though: given that the host argument is just yielded back to the caller, it seems a bit unnecessary. The name of this method also suggests that this method does something with it or uses it to determine what messages to yield.

Is there a reason you chose to pass it through? I'd probably (slightly) favor just putting a host = blah line at the call sites and then change this to each_message.

This comment has been minimized.

@JonRowe

JonRowe Aug 13, 2013
Author Member

I liked the look of passing it in and out rather than having a local variable, that was all

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Aug 13, 2013

Whats the story with #401 and #404 and this?

@xaviershay
Copy link
Member

@xaviershay xaviershay commented Aug 14, 2013

#401 is broken, if it needs to be re-implemented this patch will have to change too. I need to understand how this PR interacts with it. Didn't leave myself enough time to do that tonight, unfortunately.

end
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Aug 16, 2013
Member

I'd like to see some specs that document how .not_to receive_messages is handled. Can you add those?

# example method. They do not stash or restore existing method
# `add_stub` / `add_expectation` where it is known in advance that this
# is all that will be required of a stub, such as when passing attributes
# to the `double` example method. They do not stash or restore existing method
# definitions.

This comment has been minimized.

@myronmarston

myronmarston Aug 16, 2013
Member

As documented here, simple stubs/expectations do not stash or restore existing method defs. This is OK for test doubles (the original case @xaviershay had in mind for simple stubs) but not for partial mocks (as will be used with this feature). Can you add some specs (which should fail) about the resetting on a partial mocks, and then fix it? I suggested one possible fix here:

In #399 we'd like to use the new simple stub stuff, but I guess a simple stub on a partial mock is a "slightly less simple stub" since it needs to be reset properly. I think that should be taken care of as part of that ticket since it's not needed with what we currently have. Here's an idea for how to do that: In TestDouble#__build_mock_proxy it will instantiate a subclass of Proxy that overrides add_simple_stub to pass a support_reset: false flag to add_simple_stub. For other proxies (e.g. for a partial mock) it'll pass true for this flag, and based on the flag it'll either do configure_method or define_proxy_method; @needs_restoration = false. I think that'll work.

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Aug 19, 2013

@myronmarston I've added in specs covering what happens when you attempt to use the negative (I'm disallowing it) but I'm having trouble consistently getting a partial mocking failure. What should trigger it? I don't want to create order dependant specs...

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Sep 3, 2013

Ping! Any advice?

end
rescue RSpec::Mocks::MockExpectationError => error
error.backtrace.insert(0, @backtrace_line)
Kernel::raise error
end

This comment has been minimized.

@myronmarston

myronmarston Sep 4, 2013
Member

The logic in this method (particularly the rescue and backtrace insertion bit) is very similar to what's already here:

def verify_messages_received
generate_error unless expected_messages_received? || failed_fast?
rescue RSpec::Mocks::MockExpectationError => error
error.backtrace.insert(0, @expected_from)
Kernel::raise error
end

Can some common logic be extracted?

This comment has been minimized.

@JonRowe

JonRowe Sep 9, 2013
Author Member

Refactored

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Sep 9, 2013

I'll squash this before we merge it but I'm leaving the history until I'm ready for that :)


# Insert original locations into stacktraces
# @api private
class BacktrackRestore

This comment has been minimized.

@myronmarston

myronmarston Sep 9, 2013
Member

BacktrackRestore is a very odd name. I have no idea what the name has to do with what the code below does...

I don't have a better name idea yet, though :(.

This comment has been minimized.

@JonRowe

JonRowe Sep 9, 2013
Author Member

I've changed to InsertOntoBacktrace.line

@coveralls
Copy link

@coveralls coveralls commented Sep 9, 2013

Coverage Status

Coverage decreased (-0.06%) when pulling c3bdeff on receive_messages into 3ac6f4e on master.

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Sep 9, 2013

Ok, this now resets partial mocks properly (it was actually a fairly trivial change as we have a separate PartialMockProxy to handle this).

One thing I haven't done, and this is deliberate cause my head hurts just thinking about it, is handle resetting 'any_instance' stubs/expectations, because they are already reset entirely separately from everything else, and I'm using the existing stub/expectation functionality for those (it's not new code).

They already don't reset cleanly, so I figure that's a new issue.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 10, 2013

They already don't reset cleanly, so I figure that's a new issue.

That's news to me. Can you open an rspec-mocks issue includes a code snippet that demonstrates the problem?

myronmarston added a commit that referenced this pull request Sep 12, 2013
Allow multiple message allowances/expectations via `receive_messages`
@myronmarston myronmarston merged commit 69954e9 into master Sep 12, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@myronmarston myronmarston deleted the receive_messages branch Sep 12, 2013
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 12, 2013

Merged. Thanks @JonRowe!

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Sep 12, 2013

Weren't we going to squash this? :P

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Sep 12, 2013

Oh yeah.....too late now, I guess.

@JonRowe
Copy link
Member Author

@JonRowe JonRowe commented Sep 12, 2013

Yup, oh well :)

@rosenfeld
Copy link

@rosenfeld rosenfeld commented Sep 12, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.