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

Pass the instance to any instance stubs. #351

Merged

Conversation

penelopezone
Copy link
Member

@penelopezone penelopezone commented Jul 7, 2013

Related to #175

This passes the instance for stubs, I'm not sure what to do for expectations: passing the instance breaks with. I'm going to continue to fiddle but wanted to get this out early to get feedback :)

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@coveralls
Copy link

coveralls commented Jul 7, 2013

Coverage Status

Changes Unknown when pulling 6287f7a on samphippen:pass-instance-to-any-instance-stubs into * on rspec:master*.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@penelopezone
Copy link
Member Author

penelopezone commented Jul 7, 2013

Now passes to should receive expectations too :)

@coveralls
Copy link

coveralls commented Jul 7, 2013

Coverage Status

Changes Unknown when pulling a37e021 on samphippen:pass-instance-to-any-instance-stubs into * on rspec:master*.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@coveralls
Copy link

coveralls commented Jul 7, 2013

Coverage Status

Changes Unknown when pulling 4cb7c1a on samphippen:pass-instance-to-any-instance-stubs into * on rspec:master*.


def pass_instance_to_any_instance_stubs=(arg)
@pass_instance_to_any_instance_stubs = arg
end
Copy link
Member

@myronmarston myronmarston Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yield_instance_from_any_instance_implementation_blocks is a better name. What do you think?

@myronmarston
Copy link
Member

myronmarston commented Jul 9, 2013

This is a good start, but I think there's a simpler way to implement this:

  • Update MessageExpectation#initialize to store @method_double.object in an instance variable. (Or consider storing the method double itself, and changing the message reader to delegate to method_double.method_name). Either way, the point is to make MessageExpectation able to add the object instance to the yielded args as needed.
  • Add a method to the MessageExpectation fluent interface that instructs it to include the object instance in the yielded args. (Maybe something like yield_receiver_to_implementation_block?)
  • Change ExpectationChain#create_message_expectation_on to call yield_receiver_to_implementation_block if the config option is set. This removes the need for the hash munging and the if/else there.
  • Add the object instance to the front of the list of args passed to the inner_implementation_action block (I'll leave it to you to decide how to do that; there are a few ways to do so).

I haven't worked out all the details of this alternate implementation, but it feels cleaner to me intuitively. Among other things, the code you added to matches? can be removed, as it doesn't feel like it belongs there. Thoughts?


after(:each) do
RSpec::Mocks.configuration.pass_instance_to_any_instance_stubs = @orig_pass
end
Copy link
Member

@myronmarston myronmarston Jul 9, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than putting this here, what do you think about creating a with isolated configuration shared context? To me, isolating configuration seems like a cross-cutting concern we'll want to use elsewhere. Also, I recommend managing the isolation of the configuration by making that shared example group clear the RSpec::Mocks::Configuration instance -- that way, it isolates any config change, as opposed to just this one config option. Thoughts?

@myronmarston
Copy link
Member

myronmarston commented Jul 9, 2013

What do you think about making the config option default to to true in RSpec 3? I consider it an oversite that we didn't yield the instance to begin with. If we default to false and a user needs the instance in an implementation block, they'll need to change the config option to enable this feature, and then deal with the specs that breaks, and then they'll get to do what they wanted to do in the first place. It seems like a better default to have it set to true, and then there's no migration pain when the user needs this.

For the 2.x => 3.0 upgrade, we can backport this to 2.99, with it defaulted to false there, and with logic that prints a deprecation warning if the user passes an implementation block to any_instance, instructing them to set the option explicitly to false (so they can upgrade to 3.0 w/o a problem).

Thoughts?

@JonRowe
Copy link
Member

JonRowe commented Jul 11, 2013

I like @myronmarston's plan, but won't it force most people to have to deal with this on upgrade?

@myronmarston
Copy link
Member

myronmarston commented Jul 11, 2013

It'll only force those that use a block implementation with any_instance to do so. My impression is that that's not most users.

Sent from my iPhone

On Jul 11, 2013, at 3:58 AM, Jon Rowe notifications@github.com wrote:

I like @myronmarston's plan, but won't it force most people to have to deal with this on upgrade?


Reply to this email directly or view it on GitHub.

@penelopezone
Copy link
Member Author

penelopezone commented Jul 14, 2013

@myronmarston with with do you think users should have to specify the instance is going to come in, or not? If not should I implicitly add the object to the argument matcher, or do something else?

edit: I was asking because I was having trouble getting my implementation working. This can be disregarded now unless you feel strongly against what I've implemented.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@coveralls
Copy link

coveralls commented Jul 14, 2013

Coverage Status

Coverage remained the same when pulling cf5ef39 on samphippen:pass-instance-to-any-instance-stubs into 638d3ff on rspec:master.

@coveralls
Copy link

coveralls commented Jul 14, 2013

Coverage Status

Coverage decreased (-0%) when pulling cf5ef39 on samphippen:pass-instance-to-any-instance-stubs into 638d3ff on rspec:master.

after do
RSpec::Mocks.instance_variable_set(:@configuration, orig_configuration)
end
end
Copy link
Member

@myronmarston myronmarston Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly, I'd probably just set it to nil in a before hook:

shared_context "with isolated configuration" do
  before { RSpec::Mocks.instance_variable_set(:@configuration, nil) }
end

I think that's all you need, since the config is lazily initialized.

Copy link
Member

@myronmarston myronmarston Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, scratch that. I wasn't thinking about the fact that we may want to change the config for our tests. Setting it to nil will blow those away. So nevermind.

stub.invoke(nil, *args, &block)
elsif expectation
if expectation.yield_receiver_to_implementation
args.unshift(expectation.orig_object)
end
Copy link
Member

@myronmarston myronmarston Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't feel like it belongs in mocks/proxy.rb. Notice that you are asking expectation something and then doing something with expectation.orig_object. This is all knowledge that should be internal to MessageExpectation. (And same with the stub code above).

@myronmarston
Copy link
Member

myronmarston commented Jul 15, 2013

@myronmarston with with do you think users should have to specify the instance is going to come in, or not? If not should I implicitly add the object to the argument matcher, or do something else?

with has absolutely nothing to do with this feature. with is about what arguments are passed by the caller with the message to the receiver. It is not about what the args yielded to the implementation block are. Those are two entirely different things.

I think your concern here goes away once you move the args.unshift bit out of Proxy and into MessageExpectation.

BTW, we should make sure this doesn't interfere with and_call_original -- the receiver should not be passed through as an arg to that.

@@ -0,0 +1,22 @@
require 'spec_helper'
require 'pry'
Copy link
Member

@JonRowe JonRowe Jul 15, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you meant to leave this in ;)

Sam Phippen added 2 commits Jul 18, 2013
…ions.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@penelopezone
Copy link
Member Author

penelopezone commented Jul 18, 2013

@myronmarston I'm ok with defaulting this to true in 3.0 with the deprecation warning. Do you have any more code review on this implementation?

@@ -173,6 +186,10 @@ def matches?(message, *args)

# @private
def invoke(parent_stub, *args, &block)
if yield_receiver_to_implementation_block?
args.unshift(orig_object)
end
Copy link
Member

@myronmarston myronmarston Jul 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue with doing it here is that this will caused the received to be passed to any message expectation implementation, not just a block implementation. For example, this will cause an extra arg to be passed when SomeClass.any_instance.should_receive(:foo).and_call_original is used, which will break things, I think. I think you may want to wrap inner_implementation_action w/ a proc that unshifts the args, and then forwards it on to the original proc. Thoughts?

@myronmarston
Copy link
Member

myronmarston commented Jul 18, 2013

I'm ok with defaulting this to true in 3.0 with the deprecation warning.

Let's go with that then. If nothing else, changing it to true here should reveal if this breaks anything (such as and_call_original, as I suspect it will).

@coveralls
Copy link

coveralls commented Jul 19, 2013

Coverage Status

Coverage remained the same when pulling 7feb8a1 on samphippen:pass-instance-to-any-instance-stubs into 6770f2f on rspec:master.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@penelopezone
Copy link
Member Author

penelopezone commented Jul 25, 2013

@myronmarston I made this the default, all the specs/cukes pass. I specifically wrote a spec to deal with and_call_original which seems to be passing. (not in the most recent commit)

@coveralls
Copy link

coveralls commented Jul 25, 2013

Coverage Status

Coverage decreased (-0%) when pulling 7293bfa on samphippen:pass-instance-to-any-instance-stubs into 6770f2f on rspec:master.

@yield_instance_from_any_instance_implementation_blocks = true
end

attr_accessor :yield_instance_from_any_instance_implementation_blocks
Copy link
Member

@myronmarston myronmarston Jul 25, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • It would be nice if this was a predicate, so that it's clear that it's a query message that returns true/false rather than a command method (the fact that it starts with yield_... makes it sound like a command).
  • On MessageExpectation, you phrased it as yield_receiver_to_implementation, so for consistency, I'm thinking this should be yield_receiver_to_any_instance_implementation_blocks= and yield_receiver_to_any_instance_implementation_blocks?. I like using the word receiver for the object (rather than using instance twice in the name), and it's more accurate to say the receiver is yielded to the block rather than from the block.

@myronmarston
Copy link
Member

myronmarston commented Jul 25, 2013

Well done, @samphippen! I left a comment about the naming of the config option but this needs a changelog entry, but otherwise it's good to go.

Sam Phippen added 2 commits Jul 25, 2013
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@coveralls
Copy link

coveralls commented Jul 25, 2013

Coverage Status

Coverage decreased (-0%) when pulling a4f908f on samphippen:pass-instance-to-any-instance-stubs into 6770f2f on rspec:master.

@penelopezone
Copy link
Member Author

penelopezone commented Jul 26, 2013

I'm going to go ahead and merge this, then (later today) squash this for 2-99 and turn the option off by default and make it issue a deprecation warning.

penelopezone pushed a commit that referenced this pull request Jul 26, 2013
…-stubs

Pass the instance to any instance stubs.
@penelopezone penelopezone merged commit ad3bf96 into rspec:master Jul 26, 2013
@penelopezone penelopezone deleted the pass-instance-to-any-instance-stubs branch Jul 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants