Prevent errors on stub removal #402

JonRowe · 2013-08-10T05:02:41Z

Currently the any_instance stub functionality doesn't remove stubs from existing instances, this is the start of a fix for #397 but potentially represents a change in behaviour. We'd also need to apply this to re-stubbing I think...

Thoughts?

myronmarston · 2013-08-10T05:08:40Z

lib/rspec/mocks/space.rb

@@ -60,6 +60,10 @@ def proxy_for(object)

      alias ensure_registered proxy_for

+      def proxied_instances_of(klass)
+        proxies.select { |_, proxy| proxy.object.is_a? klass }.map { |_, proxy| proxy }
+      end


This approach is O(N). Instead, we could index the proxies by their class as they are created in a hash, and then do an O(1) lookup here. It would almost certainly be faster for any examples that use lots of mock proxies. It could potentially be slower for examples that use a small number of mock proxies, though...and that might be the more common case. Thoughts?

myronmarston · 2013-08-10T05:11:56Z

There's an odd case we need to think carefully about...what should we do when there is an instance of the class that's directly stubbed (e.g. via instance.stub, not due to klass.any_instance.stub) and any_instance.stub is also used for the same method? Should this unstub that instance as well?

JonRowe · 2013-08-17T16:26:47Z

@myronmarston I refactored this to reduce the lookup time and handle unstubbing only any instance methods, take a look?

JonRowe · 2013-08-18T10:03:22Z

Rebased to remove build failure.

myronmarston · 2013-08-20T04:37:48Z

lib/rspec/mocks/any_instance/recorder.rb

@@ -72,6 +73,10 @@ def unstub(method_name)
            raise RSpec::Mocks::MockExpectationError, "The method `#{method_name}` was not stubbed or was already unstubbed"
          end
          message_chains.remove_stub_chains_for!(method_name)
+          for proxy in ::RSpec::Mocks.proxies_of(@klass)


Why use for here rather than the more commonly seen each?

Something I took from a Dave Thomas talk, why do we always use each for
what are essentially for loops, when for is sometimes clearer, he
attributed it to cargo culting rails.

So sometimes I like to use a for ... in when I think it might read better.

I'm not overly attached to it though.

I like the consistency of using each everywhere....and that's what we tend to use.

coveralls · 2013-08-20T12:28:59Z

Coverage decreased (-0.56%) when pulling abf019f on prevent_errors_on_stub_removal into 046dd46 on master.

myronmarston · 2013-08-20T16:38:00Z

lib/rspec/mocks/space.rb

+        proxies_by_klass.inject([]) do |klass_proxies, (proxy_klass, object_ids)|
+          more_proxies =
+            if proxy_klass.ancestors.include?(klass) || klass.ancestors.include?(proxy_klass)
+              object_ids.map { |id| proxies[id] }


Given that you need the proxies, and not the object ids, why not store the proxies in the hash rather than ids? Then you don't have to do the proxies[id] lookup here.

The proxies are already being stored by id, as this was meant to be a performance tweak over the top for finding them via klass, it made sense to me to stash the smaller ids, rather than duplicating the Proxy objects.

myronmarston · 2013-08-20T17:08:32Z

Added specs and solved by iterating over classes looking at both ancestor chains (required to go from super to sub and vis versa). This hyrbrid approach should be less work than just going over all proxies, as they're essentially grouped by klass at least.

Your approach is interesting in novel :). That said, I do have some concerns:

It looks to me like there's a bug lurking here, where Subclass.any_instance.stub(:foo) has been used and there is an instance of Superclass...proxies_of(Subclass) will return the Superclass instance, right? Since klass.ancestors.include?(proxy_klass) will return truefor this case. Consider the case where you have some partial mocks that were created simply asObject.new: won't these always be returned (sinceObject` is a superclass of all classes?)
I'm concerned that this will actually make perf worse than the original thing you had. Consider that klass.ancestors.include? is an O(N) scan over the ancestors array. Some objects have very large ancestors chains. I just checked in a rails app we have what User.ancestors.count is (User is an ActiveRecord model) and it's 55! Consider that the old solution was also O(N) but was O(N) over the number of objects that have a mock proxy. I'd expect this to be smaller than 55 (a test that uses 55 test doubles and/or partial mocks seems insane).
The original code was so much simpler. This code took me awhile to understand.

All that's to say that I think it might be best to go back to this:

proxies.each_with_object do |(_, proxy), matches|
  matches << proxy if klass === proxy.object
end

(Notice that I used Class#=== rather than object.is_a?....I think it's more likely that someone will have messed with the methods on an individual object like is_a? than messed with Class#===).

coveralls · 2013-08-21T11:15:58Z

Coverage remained the same when pulling a8b330b on prevent_errors_on_stub_removal into 046dd46 on master.

JonRowe · 2013-08-21T12:00:45Z

TL;DR; I dropped it back to the simpler version.

Performance wise the other code was going over the classes registered in the mock space, so the lookup was bounded, and there would be less in that hash lookup than in proxies but I was assuming include? is implemented sanely (i.e. won't walk over all the ancestors if it finds the one it's looking for, and in the common occurrence I was expecting people to be stubbing in the top few levels)

The super/sub class behaviour of proxy_of was the point, it's not a bug, because the unstub mechanism only clobbers stub's it knows about already, so at worst there would have been a few extra no-ops. I actually expected the simpler code version to break because of the chain, but it doesn't because we check the sub class is an instance of super, rather than checking that the super class is a sub class (which this code won't do).

coveralls · 2013-08-21T15:51:46Z

Coverage decreased (-0.57%) when pulling 41217bf on prevent_errors_on_stub_removal into 046dd46 on master.

myronmarston · 2013-08-21T17:01:28Z

The super/sub class behaviour of proxy_of was the point, it's not a bug, because the unstub mechanism only clobbers stub's it knows about already, so at worst there would have been a few extra no-ops. I actually expected the simpler code version to break because of the chain, but it doesn't because we check the sub class is an instance of super, rather than checking that the super class is a sub class (which this code won't do).

I get what you're saying, but even though the one thing that uses proxies_of deals with it returning objects that are instances of superclasses, it's semantically wrong for that method to return those. The method was essentially a lie...it was really proxies_of_class_and_any_superclasses. In my experience, it's a bad idea for method to be a lie like this, as it causes confusion as soon as something else starts to use it, and is likely to cause bugs.

So thanks for changing this :).

Anyhow, I pushed two simple small improvements to some specs (let me know what you think).

From here, this needs a changelog entry. It could also benefit from being squashed into one commit: I don't know that the intermediate commits have much value, and 9 commits is a lot for what is a pretty small change.

Do you plan to backport this to 2.99 and/or 2.14?

* access proxies for any instance of klass * remove stubs from "running" instances * pass recorder into chains to register stubs * reduce lookup cost of proxies by klass * record stubs used * remove just the stubs recorded by any instance * switch from for..in to each * allow unstubing of instances that are a sub class * refactor lookup for subclass * Improve doc string. * "local instance" didn't really give me the right * sense for what this was testing. * Improve space spec. - The old spec only checked the number of proxies returned, and didn't actually check that it returned the right ones. - The old spec only tested who were instances of the given class, and did not check instances of subclasses.

coveralls · 2013-08-22T22:36:23Z

Coverage increased (+0.02%) when pulling f585362 on prevent_errors_on_stub_removal into 986e20c on master.

Prevent errors on stub removal

myronmarston · 2013-08-23T06:25:02Z

Thanks, @JonRowe! I took care of backporting it in 84ae8d1 and 21c60d6

myronmarston reviewed Aug 10, 2013
View reviewed changes

myronmarston reviewed Aug 20, 2013
View reviewed changes

myronmarston added a commit that referenced this pull request Aug 23, 2013

Merge pull request #402 from rspec/prevent_errors_on_stub_removal

590a19e

Prevent errors on stub removal

myronmarston merged commit 590a19e into master Aug 23, 2013

myronmarston deleted the prevent_errors_on_stub_removal branch August 23, 2013 06:02

myronmarston mentioned this pull request Oct 5, 2013

Stub or unstub with any_instance will not affect existing instances where stub has been invoked #397

Closed

myronmarston mentioned this pull request Apr 3, 2014

Fix any_instance so that it updates already stubbed instances #651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent errors on stub removal #402

Prevent errors on stub removal #402

JonRowe commented Aug 10, 2013

myronmarston Aug 10, 2013

myronmarston commented Aug 10, 2013

JonRowe commented Aug 17, 2013

JonRowe commented Aug 18, 2013

myronmarston Aug 20, 2013

JonRowe Aug 20, 2013

myronmarston Aug 20, 2013

coveralls commented Aug 20, 2013

myronmarston Aug 20, 2013

JonRowe Aug 20, 2013

myronmarston commented Aug 20, 2013

coveralls commented Aug 21, 2013

JonRowe commented Aug 21, 2013

coveralls commented Aug 21, 2013

myronmarston commented Aug 21, 2013

coveralls commented Aug 22, 2013

myronmarston commented Aug 23, 2013

Prevent errors on stub removal #402

Prevent errors on stub removal #402

Conversation

JonRowe commented Aug 10, 2013

myronmarston Aug 10, 2013

Choose a reason for hiding this comment

myronmarston commented Aug 10, 2013

JonRowe commented Aug 17, 2013

JonRowe commented Aug 18, 2013

myronmarston Aug 20, 2013

Choose a reason for hiding this comment

JonRowe Aug 20, 2013

Choose a reason for hiding this comment

myronmarston Aug 20, 2013

Choose a reason for hiding this comment

coveralls commented Aug 20, 2013

myronmarston Aug 20, 2013

Choose a reason for hiding this comment

JonRowe Aug 20, 2013

Choose a reason for hiding this comment

myronmarston commented Aug 20, 2013

coveralls commented Aug 21, 2013

JonRowe commented Aug 21, 2013

coveralls commented Aug 21, 2013

myronmarston commented Aug 21, 2013

coveralls commented Aug 22, 2013

myronmarston commented Aug 23, 2013