-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
Fix any_instance
so that it updates already stubbed instances
#651
Conversation
Anyone want to review this? |
Taking a look now |
@@ -25,8 +25,10 @@ class PositiveExpectationChain < ExpectationChain | |||
|
|||
def create_message_expectation_on(instance) | |||
proxy = ::RSpec::Mocks.space.proxy_for(instance) | |||
expected_from = IGNORED_BACKTRACE_LINE | |||
me = proxy.add_message_expectation(expected_from, *@expectation_args, &@expectation_block) | |||
method_name, opts = @expectation_args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I like this change, it really helped clear up what *@expectation_args
was doing.
In general it looks good. Though mocks is def one of the parts that I need to spend more time on to fully understand. Looking at the specs it's fairly clear what the intended behaviors are. Reading the new specs I feel as though this new behavior is correct. Though I may need to think on it a little more. Also, while I agree with the comments in previous issues regarding general good practice; this change feels like it makes the lib more robust. This seems inline with what I understand the goals v3 to be. |
I want to review this, but I won't have time before this evening, mind holding off merging until then? (I also want to double check this isn't going to break anything with the change in behaviour, as I have a niggling feeling about why I made the old change) |
That's fine. I'm in the car heading away for the weekend anyway. Sent from my iPhone
|
# | ||
# Proxying for the message expectation fluent interface (typically chained | ||
# off of the return value of one of these methods) is provided by the | ||
# `FluentInterfaceProxy` class below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn we have to do some terrible things to support this feature :(
(I don't mean this solution is bad, I just mean :( that we have to do it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm totally on the same page :(.
LGTM, I hate |
Fix `any_instance` so that it updates already stubbed instances
This fixes #613.
Note that this changes some behavior put in place by @JonRowe in f585362 (#402)...but I think this provides a better, more consistent/generic solution to the problem that was solving. I may be missing something, though.
The change I'm referring to is easily shown by this diff in one of the specs:
Previously,
unstub
(andand_call_original
) would not affect instances that had the method directly stubbed. This kinda sorta made sense since a stub on a particular instance is more specific than anany_instance
stub.However, I think it's far simpler and more consistent (and less code, too) to make
any_instance
affect ALL instances, even those that have been individually stubbed. It's basically "last specified stub wins", just like it is with an individual instance.What do others think about this change? (And the implementation of the fix, too, of course!)
/cc @JonRowe @cupakromer @samphippen @xaviershay