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

Add allow(...).to receive_message_chain #467

Merged
merged 1 commit into from Nov 18, 2013
Merged

Conversation

@penelopezone
Copy link
Member

penelopezone commented Nov 13, 2013

First pass. Comments on the back of a PR :)

end
end
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

This is an awful small class to get its own file. Can we find another place for it?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 13, 2013

Author Member

framework.rb?

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 14, 2013

Member

Why not leave it in receive.rb?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

I think I'm in favor of leaving it in receive.rb.

end
end
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

It looks like this file is a duplicate of receive_message_chain.rb and not even used. Can it be removed?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 13, 2013

Author Member

Yup, I biffed my git.

@@ -0,0 +1,35 @@
require "forwardable"

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

You're using forwardable below (beyond extending the module), so it looks like you can remove this require and that extension.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 13, 2013

Author Member

*not using?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

Yeah, "not using" :).

Matchers::ReceiveMessages,
Matchers::ReceiveMessageChain,
].any? { |m| m === matcher }
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

This would be more idiomatic as matcher_allowed?.

Also, seems simpler to just have a set of allowed matcher classes and then do allowed_matcher_classes.include?(matcher.class).

end
end
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

This is a very isolated unit-testy. While I often favor that approach in testing applications, within RSpec I prefer to do full integration tests of the features as users use them. The normal main downside to full integration tests is that their slow...but they are blazing fast here in RSpec (rspec-mocks' tests take about 500 ms total), and testing this feature entirely via the public API users use has two huge benefits:

  • It gives us much greater confidence the feature works.
  • It makes it much easier to refactor in the future. IMO, ReceiveMessageChain is entirely an implementation detail of this feature. I want to be free to refactor that entirely, while retaining tests that tell me if the feature is still working. Having tests that use ReceiveMessageChain directly inhibits such refactorings.

Just to give you an example of one integration bug that's not (and can't) be caught by these tests: if you look at the implementation of StubChain.stub_chain_on, you'll notice that it relies on Object#stub, which, if the user has disabled the should syntax, will raise a NoMethodError. The tests here are incapable of detecting such an integration error.

end
"""
When I run `rspec spec/chained_messages.rb`
Then the output should contain "1 example, 0 failures"

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

These cukes look like they are in the wrong file. This file is focused on expect, not allow.

@myronmarston
Copy link
Member

myronmarston commented Nov 13, 2013

I don't see anything here for:

expect(k).to receive_message_chain(...)
allow_any_instance_of(k).to receive_message_chain(...)
expect_any_instance_of(k).to receive_message_chain(...)
@recorded_customizations = []
end

[:and_return, :and_throw, :and_raise, :and_yield].each do |msg|

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

I think you're missing and_call_original.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 13, 2013

Author Member

stub chain does not work with and_call_original:

https://gist.github.com/samphippen/7458414

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 13, 2013

Member

Looks like you found a bug in stub_chain that we should fix. In your example, a is not test double. Also we should fix the wording so it says "pure test double" vs "partial double" rather than using mock in the message.

@myronmarston
Copy link
Member

myronmarston commented Nov 13, 2013

Also, what about block implementations? I don't see any specs showing that working.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 14, 2013

Also, what about block implementations? I don't see any specs showing that working.

I've now added these.

@@ -2,7 +2,7 @@
require 'delegate'

describe "and_call_original" do
context "on a partial mock object" do
context "on a partial double" do

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 14, 2013

Member

I find "partial double" confusing here, how about "on a method double" instead

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

What do you find confusing about the term "partial double"? "MethodDouble" is an internal class within rspec-mocks, but it's not a concept we expose publicly and this context isn't really about that...

This comment has been minimized.

Copy link
@JonRowe

JonRowe Nov 14, 2013

Member

A test double to me is an entirely fake object. A method double indicated we're faking only one method. I thought method double was a more widely known concept than just rspec?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Hmm, I've never heard it used outside of RSpec, but I don't have much exposure to using test doubles outside of an RSpec context.

Anyhow, in #444 the term we decided on for the new config option was "partial double", so we should be consistent here. If we want to change how we refer to the concept, we can, but that is a bigger issue that we should address in a separate PR if we do address it. For now, being consistent with that naming is best, I think.

@JonRowe
Copy link
Member

JonRowe commented Nov 14, 2013

Good work @samphippen! Will need a change log entry of course and I'd like to see these commits squashed?

Matchers::ReceiveMessages,
Matchers::ReceiveMessageChain,
]
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Can this be moved into a constant? As it is implemented now, every time it is called in allocates a new array object, which seems wasteful, given that conceptually, it's a constant.

allow(object).to receive_message_chain(:to_a, :length) { 3 }

expect(object.to_a.length).to eq(3)
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

It would be good to have a spec that shows that { } takes precedence over do...end when both block forms are given (since the curly-brace block binds directly to receive_message_chain but do...end binds to to).

@@ -118,8 +118,8 @@ def raise_wrong_arity_error(args_to_yield, arity)

# @private
def raise_only_valid_on_a_partial_mock(method)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Can you change the method name to ..._partial_double as well?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

good spot! (regex replacement fail :))

@@ -20,11 +20,11 @@ def stub_chain
matching_stub.invoke(nil).stub_chain(*chain, &block)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

The call to stub_chain here will fail if used with the :should syntax disabled. Would be good to start with a failing test, then fix it to not call that anymore. Reading the code, it looks like this line will get hit with a spec like this:

allow(obj).to receive(:foo).and_return(double)
allow(obj).to receive_message_chain(:foo, :bar:, :bazz)

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

@myronmarston So all the specs now run with only the expect syntax and they're passing without changing this. 😕

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Have you tried adding an example like I outlined there? Alternately, if you want to figure out the code path that needs that, try changing that line to raise "boom" and see what fails -- based on that, you can then write an example using the new syntax that hits this line.

end

context "when only the expect syntax is enabled" do
include_context "with syntax", :expect

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

I think I'd prefer to see the :should syntax disabled for all of the specs in this file. I can't think of a way that having the :should syntax enabled would break it, but there are many branches and code paths to the implementation of RSpec::Mocks::StubChain and it would give me greater confidence that it works for all of them if :should is disabled for all the specs in this file. Or, if you do want to test that it works with :should enabled, that's fine -- just flip it so that there's only one test where it is enabled and all the rest where is is disabled.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

👍

end
end
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Some other things to consider testing here:

  • expect().to receive_message_chain -- this is a feature that the old syntax lacked, but I don't see a reason not to support it. It's one thing I like about the new expect/allow + matcher system: we easily get feature parity for both message expectations and message allowances. That said, if it's a bunch of work to get this to work, I'm not opposed to disallowing it....but I'd want a test here that documents that fact. As it stands now, it's completely undocumented what happens when you use expect with receive_message_chain.
  • receive_message_chain("foo.bar.bazz") -- this is one of the things stub_chain supports and we may as well support it. I think it should "just work" but a test to confirm and document would be great.
  • receive_message_chain(:foo, :bar, :bazz => "return value") -- this works for stub_chain (see stub_chain_spec.rb) and should either work here or be documented as not working.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

any instance expect proved to be difficult, everything else exists now.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Thanks! One other thing I forgot to mention: the negative case (e.g. allow().not to receive_message_chain). I think you already have code to prevent this but specs to document the behavior would be good. Given you already have the code in place for it, it'd be good to "break" it somehow to confirm the tests you add for this can properly fail with a useful failure message.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

I will add a spec for this later :)

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 15, 2013

Author Member

done

module RSpec
module Mocks
module Matchers
class ReceiveMessageChain

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

A big part of being SemVer compliant is being explicit about what is part of the public API and what is not. Every class should have yard comments documenting that. I would make this @api private since it's not intended to be instantiated directly by end users.

@@ -124,6 +124,10 @@ def receive_messages(message_return_value_hash)
matcher
end

def receive_message_chain(*messages, &block)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

This method needs yard docs added for it. See the big block of comments at the bottom of the file.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

Now added.


private

attr_reader :stubber

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Now that you are not injecting a test double for the stubber in your tests here I think it makes less sense to support the stubber being injectable. Thoughts on removing this reader and the stubber arg from initialize, opting to just directly reference StubChain instead?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

I'm sort of on the fence about this one. I really like DI if it's usable. I do, however, foresee this class being very coupled to stubchain (or at least it's interface) by nature of what it does.

I think directly referencing StubChain here is fine though.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

I like DI a lot, too, but my general rule of thumb is to only add it if I use it in one place. It's not being used anywhere here, so it feels like overhead to me.

when Matchers::ReceiveMessages
raise_negation_unsupported(method_name, matcher)
when Matchers::ReceiveMessageChain
raise_negation_unsupported(method_name, matcher)

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

These last two when clauses can be combined into one:

when Matchers::ReceiveMessages, Matchers::ReceiveMessageChain
  raise_negation_unsupported(method_name, matcher)

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

👍



module RSpec::Mocks::Matchers
describe "allow(...).to receive_message_chain(...)" do

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

This example group describes more than just allow(...).to receive_message_chain. Should this just be describe "receive_message_chain" do?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

👍

expect {
expect_any_instance_of(Object).to receive_message_chain(:to_a, :length => 3)
}.to raise_error(/does not work with `expect_any_instance_of`/)
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

What is the issue with expect_any_instance_of? IMO, it seems inconsistent that allow, expect and allow_any_instance_of all support this but expect_any_instance_of doesn't. It would be more consistent to get it to work or to disallow it with expect as well.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

I worked it out on a second look. Something about that code was confusing, but I think I can add it now :)

🍺

expect(obj.a).to eq 1
expect(obj.b).to eq 2
end
end

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member
  • What prompted adding this?
  • The example group this is in is allow(...).to receive_messages(:a => 1, :b => 2) -- but this is expect_any_instance_of.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 14, 2013

Author Member

just a git fail plus getting a little confused whilst looking at another matcher and testing an assumption.

# indicate real problems (think fluent interfaces), `stub_chain` still
# results in brittle examples. For example, if you write
# `foo.stub_chain(:bar, :baz => 37)` in a spec and then the
# implementation calls `foo.baz.bar`, the stub will not work.

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

This description mentions stub_chain but should mention receive_message_chain instead.

# double.foo.bar # => :baz
#
# # Common use in Rails/ActiveRecord:
# Article.stub_chain("recent.published") { [Article.new] }

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 14, 2013

Member

Ditto here: this shouldn't mention stub_chain.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 15, 2013

@myronmarston I have to get on a plane now, but I think I've addressed all the feedback. Can you take another look through this and tell me what you think when it goes green?

Thanks 😄

I'll add a changelog in ~8 hours when I get off the plane.

penelopezone pushed a commit that referenced this pull request Nov 15, 2013
Sam Phippen
@penelopezone
Copy link
Member Author

penelopezone commented Nov 17, 2013

👍 I think it's organized better now. Do you like it better or worse?

I'm down with this. I particularly like how chain_on got pushed into one place.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 17, 2013

@myronmarston I've updated based on your feedback.

I've noticed that if you've set an expectation/allowance before you created a chain, and that expectation/allowance returns nil you get a stubbing method on nil warning. I'm not sure how you feel about that, but I'm pretty sure it's an inevitable consequence.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 17, 2013

@myronmarston can you take a look at this please?

chain_on(matching_stub.invoke(nil), *chain, &@block)
elsif matching_expectation = find_matching_expectation
chain.shift
matching_expectation.actual_received_count -= 1

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 18, 2013

Member

It's unclear to me why this line is here...can you explain?

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 18, 2013

Author Member

The expectations in the tests were failing due to the expect() setting up an expectation that exactly one call is made. I realised this is because we invoke the first expectation once, and then when the entire chain is called, that first expectation gets called a second time. I guess to me, the least surprising behaviour is that the tests as they are pass. I guess I could increment the expected received count instead?

This comment has been minimized.

Copy link
@myronmarston

myronmarston Nov 18, 2013

Member

I get it now. Instead, what do you think about adding a invoke_without_changing_received_count method to MessageExpectation? to me, it feels hacky to manipulate the received counts directly.

This comment has been minimized.

Copy link
@penelopezone

penelopezone Nov 18, 2013

Author Member

@myronmarston that's a really good call. I love how you always come up with this stuff.

@myronmarston
Copy link
Member

myronmarston commented Nov 18, 2013

I've noticed that if you've set an expectation/allowance before you created a chain, and that expectation/allowance returns nil you get a stubbing method on nil warning. I'm not sure how you feel about that, but I'm pretty sure it's an inevitable consequence.

It's the correct behavior. That said, you could avoid it in your test by doing something like:

allow(obj).to receive(:foo).and_return(double)
allow(obj).to receive_message_chain(:foo, :bar, :bazz)

...because then it would be stubbing the returned double rather than the implicitly returned nil.

This is looking great, @samphippen :).

@mhenrixon
Copy link

mhenrixon commented Nov 18, 2013

Woohoo this is coming along nicely! Good job :) 👍

@penelopezone
Copy link
Member Author

penelopezone commented Nov 18, 2013

@myronmarston thoughts on matching_expectation.invoke_without_incrementing_received_count and the implementation there in?

@myronmarston
Copy link
Member

myronmarston commented Nov 18, 2013

LGTM. Let's squash this down and merge it!

BTW, if it's not too much trouble to backport this to 2.99, that would be nice, as I suspect that transpec will be gain the ability to do the conversion and it would be nice to convert to this as folks upgrade. Not essential though.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 18, 2013

@myronmarston /me wipes sweat of brow. Well that was fun. I'll squash it down to one commit, and then cherry pick it across to 2-99 assuming that's sufficiently easy.

penelopezone pushed a commit that referenced this pull request Nov 18, 2013
Add allow(...).to receive_message_chain
@penelopezone penelopezone merged commit 4662eb0 into master Nov 18, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@penelopezone penelopezone deleted the receive-chained-messages branch Nov 18, 2013
@mhenrixon
Copy link

mhenrixon commented Nov 18, 2013

woohooo!! This makes me a really happy camper 👍

@penelopezone
Copy link
Member Author

penelopezone commented Nov 18, 2013

@myronmarston I had a crack at reconciling this with 2-99 and failed to merge it properly. I'll take another go tomorrow, but this may be unsurprising: these things are now quite out of sync.

@penelopezone
Copy link
Member Author

penelopezone commented Nov 21, 2013

@myronmarston couldn't merge this into 2-99. I think we'll just leave it for 3.0.0?

@mhenrixon
Copy link

mhenrixon commented Nov 21, 2013

What happened to:

expect(something).to receive_message_chain('whatever.something').with(?)

?

@myronmarston
Copy link
Member

myronmarston commented Nov 21, 2013

@myronmarston couldn't merge this into 2-99. I think we'll just leave it for 3.0.0?

I'm fine with that.

@mhenrixon -- did stub_chain support with? (Shows you how rarely I've used that feature). What are it's semantics? Is it just constraining the last message?

@mhenrixon
Copy link

mhenrixon commented Nov 21, 2013

Oh sorry about that I thought you guys could read my mind!

No stub_chain didn't support this but stub_chain wasn't an expectation either so to make it truly useful I would expect that .with(args) just constrains the last message. Think @samphippen already suggested this in the originating issue?

@myronmarston
Copy link
Member

myronmarston commented Nov 21, 2013

No stub_chain didn't support this but stub_chain wasn't an expectation either so to make it truly useful I would expect that .with(args) just constrains the last message. Think @samphippen already suggested this in the originating issue?

The reason we now support expect(...).to receive_message_chain is because the new allow vs expect + a matcher approach made it fall out naturally. We have a lot of other stuff to work on for RSpec 3 and I'm not convinced that adding with is a good idea. receive_message_chain is specifying multiple messages. There are multiple ways to interpret what with means. For example, someone could reasonably think it works like this:

expect(obj).to receive_message_chain("foo.bar.bazz").with(1).with(2).with(3)
obj.foo(1).bar(2).bazz(3)

...or like this:

expect(obj).to receive_message_chain("foo.bar.bazz").with(1)
obj.foo(1).bar(1).bazz(1)

...or like this:

expect(obj).to receive_message_chain("foo.bar.bazz").with(1)
obj.foo(1).bar.bazz

...or like this:

expect(obj).to receive_message_chain("foo.bar.bazz").with(1)
obj.foo.bar.bazz(1)

It's ambiguous (or at least, reasonable to interpret in different ways).

If you care what arguments are received, then use the existing tools available to you.

@mhenrixon
Copy link

mhenrixon commented Nov 21, 2013

Could you guide me to how I could achieve that myself now that all the terribly tough work has been done by you guys already? Could I hack that together myself somehow?

@myronmarston
Copy link
Member

myronmarston commented Nov 21, 2013

allow(obj).to receive_message_chain(:foo, :bar).and_return(double)
expect(obj.foo.bar).to receive(:bazz).with(/adfads/)
@jsmestad
Copy link

jsmestad commented Jul 17, 2014

@myronmarston does receive_message_chain still exist?

@myronmarston
Copy link
Member

myronmarston commented Jul 17, 2014

@myronmarston does receive_message_chain still exist?

Yes. In fact, it's only been in one release: 3.0 (which is also the most recent release). We have no plans to ever remove it.

@stephan-nordnes-eriksen
Copy link

stephan-nordnes-eriksen commented Feb 18, 2015

@myronmarston the receive_message_chain is really nice, but I don't see how useful it is if you can't actually, somehow, test the arguments. Would it not be possible to make the syntax something like this? expect(obj).to receive(:method_name).with("data").receive(:method_two).with("data2").and_return("result")

Example of usecase:

Say I want to test a single-fire object like MessageClass.new("data").send_to("username"). Of course you can use spy on the MessageClass and stub the new method, and then run an expect on the spy. But it is tedious and would be much nicer to have in a chain.

Also, there are probably some piping libraries that could benefit from this as well, think: dataset.filter("whatever").order.first(20).

@myronmarston
Copy link
Member

myronmarston commented Feb 18, 2015

@stephan-nordnes-eriksen -- please open a new issue. Feature requests posted at the bottom of a merged pull request tend to get lost in the shuffle.

@stephan-nordnes-eriksen
Copy link

stephan-nordnes-eriksen commented Feb 18, 2015

Yea, I agree, but I don't want to create a feature request if it is something that won't be possible to implement.

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

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