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

Implement and_wrap_original #762

Merged
merged 3 commits into from Aug 24, 2014

Conversation

Projects
None yet
2 participants
@JonRowe
Member

JonRowe commented Aug 20, 2014

This probably needs more specs and needs features but this converts and_call_original into and_wrap_original then uses and_wrap_original to implement the old and_call_original (thus nicely proving it works!).

Closes #650.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 21, 2014

Thanks, @JonRowe, I plan to review this in the morning.

#
# @example
#
# expect(api).to receive(:large_list).and_wrap_original do |original_method|

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

Would be good for this to show that the block receives the method args and a block (if there is one).

end
# Tells the object to delegate to the supplied block but passing the
# original unmodified method for your use.

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

This is accurate but I think the use case is more clear with something like:

# Decorates the stubbed method with the supplied block. The original method
# is passed to the block as the first argument (before any method call args) so
# that you can delegate to it while being able to change what args are passed to
# it and/or mutate the return value.
@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 21, 2014

The implementation looks great. I agree that it needs to have specs/cukes fleshed out more and it needs a changelog entry.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Aug 21, 2014

Ping, added some specs and a feature.

Feature: Wrapping the original implementation
Use `and_wrap_original` to make a partial double response as it normally would. This can
be useful when you want to expect a message without interfering with how it responds. You

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

These sentences are confusing and inaccurate. It doesn't make it respond as it normally does...it makes it respond with whatever your block does (but your block has easy access to the original method). You use and_wrap_original (over and_call_original) specifically because you want to interfere with how it responds.

This comment has been minimized.

@JonRowe

JonRowe Aug 23, 2014

Member

I knew I forgot to change something :P

}.to change { instance.results.size }.from(100).to(10)
end
it "will raise a name error if the method does not exist" do

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

s/will raise/raises/

value = nil
allow(instance).to receive(:results).and_wrap_original { |m| value = m }
instance.results
expect(value).to be_a Method

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

You could make this expectation stronger:

original_method = instance.method(:results)
allow(instance).to receive(:results).and_wrap_original { |m| value = m }
instance.results
expect(value).to eq(original_method)
expect(value).to eq block
end
it "will ignore previous stubs" do

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

s/will ignore/ignores/

allow(instance).to receive(:results) { "results" }
allow(instance).to receive(:results).and_wrap_original { |m| m.call }
expect(instance.results).to_not eq "results"
end

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

Is it worth adding a spec showing how with can be used to still specify a canned response for specific arguments, like the cuke has?

@@ -0,0 +1,57 @@
require 'delegate'

This comment has been minimized.

@myronmarston

myronmarston Aug 21, 2014

Member

Why are you requiring this?

@JonRowe JonRowe force-pushed the and_wrap_original branch 3 times, most recently from 51c4a05 to cb284c5 Aug 23, 2014

@JonRowe JonRowe force-pushed the and_wrap_original branch from cb284c5 to a05ebbf Aug 23, 2014

@JonRowe

This comment has been minimized.

Member

JonRowe commented Aug 23, 2014

@myronmarston I addressed your feedback, waiting for travis though

@myronmarston

This comment has been minimized.

Member

myronmarston commented Aug 23, 2014

Needs a changelog, then merge away once green.

Changelog for #762
[skip ci]

JonRowe added a commit that referenced this pull request Aug 24, 2014

Merge pull request #762 from rspec/and_wrap_original
Implement and_wrap_original

@JonRowe JonRowe merged commit fd9cd38 into master Aug 24, 2014

@JonRowe JonRowe deleted the and_wrap_original branch Aug 24, 2014

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