Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Externalize methods and state that was previously held on all objects. #250

Merged
merged 7 commits into from Mar 28, 2013

Conversation

Projects
None yet
4 participants
Owner

myronmarston commented Mar 25, 2013

Externalize methods and state that was previously held on all objects.

  • Remove @mock_proxy ivar
  • Remove rspec_verify, rspec_reset, __mock_proxy and __remove_mock_proxy methods.

This is a refactoring that I'm doing to pave the way for the new expect
syntax (#153). As that syntax "wraps" objects in order to mock or stub
them, we need to externalize these methods and state. The methods directly
on the object (e.g. stub and should_receive) will simply delegate to
this external logic.

This refactoring had some nice side benefits:

  • No need for the hacky YAML fix. It was only needed because we were storing
    @mock_proxy on the mocked object.
  • AnyInstance no longer needs to much with dup; again, this was only needed
    because it dup'd the @mock_proxy ivar.
  • The Marshal extension gets significantly simpler (again, due to not
    storing @mock_proxy on the object anymore).

Rather than storing the mock proxies on the objects, we are now storing
them in a hash, keyed by object_id. This is pretty simple and consistent,
but could be problematic for objects that muck with object_id. That method
seems a bit sacred, though, so I'm not too worried about it.

If there are concerns about the object_id approach I've used here, an alternate way we can consider is defining a singleton __mock_proxy method on an object when it is mocked or stubbed. I think I like the object_id approach better, though.

@dchelimsky / @alindeman -- I could really use your eyes on this. This is a pretty massive refactoring that potentially has far reaching effects that I haven't considered, so I would really love your feedback on this!

myronmarston added some commits Mar 24, 2013

Extract rspec_reset/rspec_verify into a spec helper method.
This refactoring is in prep for removing rspec_reset/rspec_verify
from every object; by extracting it into a helper method it
will make it easier to change w/o breaking specs.
Externalize methods and state that was previously held on all objects.
- Remove `@mock_proxy` ivar
- Remove `rspec_verify`, `rspec_reset`, `__mock_proxy` and `__remove_mock_proxy` methods.

This is a refactoring that I'm doing to pave the way for the new expect
syntax (#153). As that syntax "wraps" objects in order to mock or stub
them, we need to externalize these methods and state. The methods directly
on the object (e.g. `stub` and `should_receive`) will simply delegate to
this external logic.

This refactoring had some nice side benefits:

- No need for the hacky YAML fix. It was only needed because we were storing
  `@mock_proxy` on the mocked object.
- AnyInstance no longer needs to much with `dup`; again, this was only needed
  because it dup'd the `@mock_proxy` ivar.
- The Marshal extension gets significantly simpler (again, due to not
  storing `@mock_proxy` on the object anymore).

Rather than storing the mock proxies on the objects, we are now storing
them in a hash, keyed by object_id.  This is pretty simple and consistent,
but could be problematic for objects that muck with `object_id`. That method
seems a bit sacred, though, so I'm not too worried about it.
Fix specs on 1.8.7.
Hashes are ordered on 1.9 but not on 1.8. Thus, we cannot count on
the order the mock proxies will be reset or verified since they are
held in a hash.

@dchelimsky dchelimsky and 1 other commented on an outdated diff Mar 25, 2013

lib/rspec/mocks/any_instance.rb
end
end
- def restore_dup
- if method_defined?(:__rspec_original_dup)
- class_eval do
- alias_method :dup, :__rspec_original_dup
- remove_method :__rspec_original_dup
- remove_method :__rspec_dup
- end
- end
+ def self.reset_all
+ tracked_klasses.clear
+ end
+
+ def self.tracked_klasses
@dchelimsky

dchelimsky Mar 25, 2013

Owner

Why not just tracked_classes?

@myronmarston

myronmarston Mar 25, 2013

Owner

No particular reason; I'm just in the habit of using klass in the place of class since class is a keyword. Here as part of a longer variable name tracked_classes is fine. I'll update it.

@dchelimsky dchelimsky and 1 other commented on an outdated diff Mar 25, 2013

lib/rspec/mocks/methods.rb
- def __mock_proxy
- @mock_proxy ||= begin
- mp = if TestDouble === self
- Proxy.new(self, @name, @options)
- else
- Proxy.new(self)
- end
-
- Serialization.fix_for(self)
- mp
- end
- end
-
- def __remove_mock_proxy
- @mock_proxy = nil
+ ::RSpec::Mocks.space.mock_proxy_for(self).received_message?(message, *args, &block)
@dchelimsky

dchelimsky Mar 25, 2013

Owner

I used mock_proxy when it was part of every object in order to avoid a class of potential conflicts. Now that this is nicely decoupled, how about removing the mock_ prefix? Then if we attach proxy_for directly to Mocks we get:

::RSpec::Mocks.proxy_for(self).received_message?(message, *args, &block)
@dchelimsky

dchelimsky Mar 25, 2013

Owner

By "attach" I mean "redirect to" i.e. it would still be implemented on Space.

@myronmarston

myronmarston Mar 25, 2013

Owner

I like how that reads.

@dchelimsky dchelimsky commented on an outdated diff Mar 25, 2013

lib/rspec/mocks/space.rb
@@ -24,10 +32,20 @@ def expectation_ordering
@expectation_ordering ||= OrderGroup.new
end
- private
+ def mock_proxy_for(object)
+ mock_proxies.fetch(object.object_id) do
+ mock_proxies[object.object_id] = if TestDouble === object
@dchelimsky

dchelimsky Mar 25, 2013

Owner

I find this hard to read. I'd either indent the lines below so they are aligned with the if or make it a one-liner:

mock_proxies[object.object_id] = TestDouble === object ? object.__build_mock_proxy : Proxy.new(object)

Owner

dchelimsky commented Mar 25, 2013

I've made a few comments on naming and readability, but overall I think this is a fantastic change. I don't see any issues, but this is a big enough change that I'd urge you to do a release candidate and give it a week or two for early adopters to discover any issues we're not seeing.

Owner

myronmarston commented Mar 25, 2013

I don't see any issues, but this is a big enough change that I'd urge you to do a release candidate and give it a week or two for early adopters to discover any issues we're not seeing.

That sounds wise. Thanks for the suggestion. I'll hold off on releasing this until I actually have the syntax we've discussed in #153 done. This is just a prepatory refactoring.

Owner

dchelimsky commented Mar 25, 2013

I'll hold off on releasing this until I actually have the syntax we've discussed in #153 done. This is just a prepatory refactoring.

Understood, but I'd recommend merging sooner than later. You can still do an RC if you release this code before adding the new syntax (I've done RC's in the past (sometimes) when there were invasive refactorings even though there were no external changes), and you'll increase the opportunity to expose issues and reduce the cost of deferring the merge.

Owner

myronmarston commented Mar 25, 2013

Understood, but I'd recommend merging sooner than later. You can still do an RC if you release this code before adding the new syntax (I've done RC's in the past (sometimes) when there were invasive refactorings even though there were no external changes), and you'll increase the opportunity to expose issues and reduce the cost of deferring the merge.

Right, I'm planning on merging this pretty soon (want to address your comments and fix the travis build -- aparently JRuby 1.9 has some issue I need to address). I just wasn't planning on releasing an RC until the new syntax is merged as well. I think it's a lot easier to get folks to try an RC if the announcement is "rspec-mocks now supports the expect syntax; please try the RC and report issues" rather than "rspec-mocks had a massive internal refactoring that didn't (yet) add any new features".

Owner

dchelimsky commented Mar 25, 2013

I actually think that many users would relish the notion of a refactoring that reduced monkey patching :)

Owner

JonRowe commented Mar 25, 2013

👍 Myself included

myronmarston added some commits Mar 28, 2013

Make spec pending that intermittently fails on JRuby in 1.9 mode.
I've spent a few hours troubleshooting this and haven't been able
to make progress.  It's intermittent and bizarre.  For now, just
making it pending.

myronmarston added a commit that referenced this pull request Mar 28, 2013

Merge pull request #250 from rspec/externalize_methods_and_mock_state
Externalize methods and state that was previously held on all objects.

@myronmarston myronmarston merged commit 5618f72 into master Mar 28, 2013

1 check passed

default The Travis build passed
Details

@myronmarston myronmarston deleted the externalize_methods_and_mock_state branch Mar 28, 2013

myronmarston added a commit that referenced this pull request Mar 28, 2013

Contributor

alindeman commented on 210c9f0 Mar 30, 2013

Flyby review: could we use __id__ instead? Would that address your concern about objects that muck with object_id?

Owner

myronmarston replied Mar 30, 2013

Flyby review: could we use id instead? Would that address your concern about objects that muck with object_id?

I didn't even know about __id__! Not sure how I missed that...

Anyhow, I was playing around in IRB and noticed that BasicObject provides __id__ but not object_id:

➜  ~  irb
1.9.3p327 :001 > k = BasicObject.new
(Object doesn't support #inspect)
 =>
1.9.3p327 :002 > k.__id__
 => 70139647345580
1.9.3p327 :003 > k.object_id
NoMethodError: undefined method `object_id' for #<BasicObject:0x007f9551878758>
    from (irb):3
    from /Users/myron/.rvm/rubies/ruby-1.9.3-p327/bin/irb:16:in `<main>'
1.9.3p327 :004 >

So it looks like we should use __id__. I checked in IRB on 1.8.6 and noticed that method works there, as well. There's still the possibility of someone mucking with __id__ but I feel like the underscores communicate "this is off limits; don't mess with this" and an object that redefines __id__ is a poorly behaved object, IMO.

Contributor

alindeman replied Mar 31, 2013

Awesome! Exactly, what I meant (but didn't exactly say) is that I consider any object that messes with __ methods to be completely insane and unsupported.

I forgot that __id__ was on BasicObject too. Good find.

Owner

myronmarston replied Mar 31, 2013

I just pushed a PR (#259) for this :). Will merge when travis is green. (But feel free to review it if you want).

Owner

dchelimsky replied Jun 5, 2013

@myronmarston can we eliminate the serialization_spec entirely after this commit? Asking because I'd like to get rid of the warning Ruby 2.0 emits saying "syck has been removed" triggered by https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/serialization_spec.rb#L63

Owner

myronmarston replied Jun 5, 2013

@myronmarston can we eliminate the serialization_spec entirely after this commit? Asking because I'd like to get rid of the warning Ruby 2.0 emits saying "syck has been removed" triggered by https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/serialization_spec.rb#L63

I don't think we can remove it entirely. We still have our marshal fix that it is testing. We don't have any YAML-specific code in rspec-mocks, anymore, but given that it was a bug in the past I'd like to keep the YAML specs to prevent future regressions.

What do you think about putting ruby version conditionals in place so that on 2.0 the syck one is skipped?

Owner

dchelimsky replied Jun 5, 2013

I had done that locally and felt bad about it :) But if you think that's fine I'll make it so.

Contributor

alindeman commented Apr 1, 2013

I think these changes are causing rspec-rails' test suite to fail. Here's an example I've pared it down to. I haven't dug into rspec-mocks' code to figure out what's going on yet. I will try to loop back soon but thought I'd share in case you get a chance sooner.

# passes on rspec-mocks 2.13.0
# fails on rspec-mocks master
describe "foo" do
  def hello
    :original
  end

  it "replaces hello" do
    self.stub!(:hello) { :stubbed }
    expect(hello).to eq(:stubbed)
  end
end

I think calling stub! on the example is a little weird, but it does currently happen in rspec-rails' test suite. I might factor it out if I can think of better alternatives.

Note we have to use stub! as stub means double in this case.

Owner

JonRowe commented Apr 1, 2013

Internally stub! is an alias for stub... there is no implementation difference...

Contributor

alindeman commented Apr 1, 2013

Internally stub! is an alias for stub... there is no implementation difference...

Not in the context of an example group :)

self.stub refers to ExampleMethods#stub in that case.

Contributor

alindeman commented Apr 1, 2013

(FWIW, I was confused at first too ... it's an unfortunate naming collision)

myronmarston added a commit that referenced this pull request Apr 3, 2013

Externalize `any_instance` methods and state like we did mock proxies.
This is similar to #250, but doing a similar refactoring for `any_instance`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment