Prevent mutated args from failing have_received expectations #1183
Conversation
lib/rspec/mocks/proxy.rb
Outdated
| @@ -161,7 +161,7 @@ def has_negative_expectation?(message) | |||
| # @private | |||
| def record_message_received(message, *args, &block) | |||
| @order_group.invoked SpecificMessage.new(object, message, args) | |||
| @messages_received << [message, args, block] | |||
| @messages_received << [message, args.map(&:dup), block] | |||
There was a problem hiding this comment.
We can't assume it's safe to dup all the args. After all, there are a lot of common objects in Ruby that are not dupable:
irb(main):001:0> 1.dup
TypeError: can't dup Fixnum
from (irb):1:in `dup'
from (irb):1
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
irb(main):002:0> nil.dup
TypeError: can't dup NilClass
from (irb):2:in `dup'
from (irb):2
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
irb(main):003:0> :symbol.dup
TypeError: can't dup Symbol
from (irb):3:in `dup'
from (irb):3
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
irb(main):004:0> true.dup
TypeError: can't dup TrueClass
from (irb):4:in `dup'
from (irb):4
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
irb(main):005:0> false.dup
TypeError: can't dup FalseClass
from (irb):5:in `dup'
from (irb):5
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
irb(main):006:0> 1.1.dup
TypeError: can't dup Float
from (irb):6:in `dup'
from (irb):6
from /Users/myron/.rubies/ruby-2.1.7/bin/irb:11:in `<main>'
It would be nice if Ruby had some kind of "dup if you can" method, but it doesn't. The way we handled this for the change matcher (which had a similar issue) was to only dup Enumerable and String objects (except for IO objects, which are Enumerable):
That's worked pretty well for that matcher, so maybe we can apply the same strategy here. It's not perfect, but it's the best we can come up with.
There was a problem hiding this comment.
Interesting, FYI, dup is a noop for those on 2.4.2
| dbl = double_with_met_expectation(:expected_method, args) | ||
| args.clear | ||
| expect(dbl).to have_received(:expected_method).with([:expected, :args]) | ||
| end |
There was a problem hiding this comment.
Can you add a spec that uses some of the un-dupable primitives? It'd be nice to have that to demonstrate why we can't dup everything.
spec/rspec/mocks/stub_spec.rb
Outdated
| @@ -110,15 +110,17 @@ def called_methods | |||
| end | |||
|
|
|||
| def method_missing(name, *) | |||
| called_methods << name | |||
| called_methods << name.to_s | |||
There was a problem hiding this comment.
So I could reliably compare the method name, regardless of Ruby version.
JonRowe
force-pushed
the
allow_mutation_with_have_received
branch
from
d28c329
to
b9d4997
Compare
|
Green bar jruby which is failing due to unrelated reasons, care to re-review @myronmarston ? |
| end | ||
|
|
||
| # @private | ||
| def dup_if_mutable(arg) |
There was a problem hiding this comment.
Do you think it's worth refactoring this method to make it clear that we only dup, Enumerable and String. And everything else we don't dup.
In essence what I'm saying is remove the first when IO, StringIO and just refactor this code to say something like:
case arg
when Enumerable, String
arg.dup
else
arg
end
There was a problem hiding this comment.
Both IO and StringIO are also Enumerable :(
There was a problem hiding this comment.
Thanks @xaviershay, I think that should be included in the commit message.
Code comments normally expire, whereas commit messages are like living documentation for why some code was written in a certain way. Perhaps we could replace the comment with a commit message?
There was a problem hiding this comment.
eh I don't have strong feelings, but bikeshedding fwiw.. :)
For me, commit message = explain why we made changes in the first place. Comments = explain why/how a particular bit of (small) code works. I don't see how this comment would "expire" since it's hyperlocal to the code it affects. This is also a pattern we need to use multiple times in the code base [citation needed, but I know I've been bitten by it a few times].
There was a problem hiding this comment.
It's clear enough and matches how we do it elsewhere
This came up in rspec/rspec-expectations#1017 we need to duplicate args we capture for later replay onto expectations, in case they are mutated downstream. This seems like an "ok" way to fix the problem, but maybe someone can suggest a better one?