Subscribe recorders to proxies #654

fables-tales · 2014-04-06T10:56:38Z

A sketch of a fix for #652

I'm not sure using playback and then sending the message is even sensible, what do people think?

Also: there was a failing spec for me locally, but it was present before this commit. It's rspec ./spec/rspec/mocks/partial_double_spec.rb:140 # Using a partial mock on a proxy object works properly

This is based on top of Myron's branch in: #651

myronmarston · 2014-04-06T19:34:00Z

I merged my PR but the diff here is still showing that code. Can you rebase this, @samphippen?

fables-tales · 2014-04-06T19:36:40Z

Done!

JonRowe · 2014-04-07T11:37:52Z

spec/rspec/mocks/any_instance_spec.rb

-          # TODO: this shouldn't be necessary to satisfy the expectation, but is.
-          klass.new.foo(2)
+          #  # TODO: this shouldn't be necessary to satisfy the expectation, but is.
+          #  klass.new.foo(2)


Why not just remove this like below ;)

Imagine a universe where I am good at writing ruby. In that universe these
lines don't exist.

I'll fix it when I get back to london.

Sent from my phone please excuse my brevity.

On 7 Apr 2014, at 13:37, Jon Rowe notifications@github.com wrote:

In spec/rspec/mocks/any_instance_spec.rb:

@@ -426,8 +426,8 @@ def inspect
expect_any_instance_of(klass).to receive(:foo).with(2).and_return(2)
expect(instance.foo(2)).to eq(2)

# TODO: this shouldn't be necessary to satisfy the expectation, but is.

klass.new.foo(2)

# # TODO: this shouldn't be necessary to satisfy the expectation, but is.

# klass.new.foo(2)

Why not just remove this like below ;)

Reply to this email directly or view it on
GitHubhttps://github.com//pull/654/files#r11339854
.

JonRowe · 2014-04-07T11:46:36Z

This looks good to me but is currently failing due to some undocumented code:

RSpec::Mocks::AnyInstance::Recorder#notify

fables-tales · 2014-04-07T11:49:48Z

Gonna mark it Api private

Sent from my phone please excuse my brevity.

On 7 Apr 2014, at 13:46, Jon Rowe notifications@github.com wrote:

This looks good to me but is currently failing due to some undocumented
code:

RSpec::Mocks::AnyInstance::Recorder#notify

Reply to this email directly or view it on
GitHubhttps://github.com//pull/654#issuecomment-39720623
.

fables-tales · 2014-04-07T14:42:18Z

@JonRowe so you don't think the approach of doing a playback, followed by a call is quite odd here? I'm slightly worried this could have undesired side effects. Maybe @myronmarston can comment, as I said, I'm really not sure about this approach.

myronmarston · 2014-04-07T15:51:28Z

I think the playback is problematic, too. Will respond with more thoughts later.

Sent from my iPhone

On Apr 7, 2014, at 7:42 AM, Sam Phippen notifications@github.com wrote:

@JonRowe so you don't think the approach of doing a playback, followed by a call is quite odd here? I'm slightly worried this could have undesired side effects. Maybe @myronmarston can comment, as I said, I'm really not sure about this approach.

—
Reply to this email directly or view it on GitHub.

myronmarston · 2014-04-07T17:10:38Z

lib/rspec/mocks/any_instance/recorder.rb

@@ -22,6 +22,16 @@ def initialize(klass)
          @expectation_set = false
        end

+        # @private
+        def notify(object, message, args, blk)


I think it'd be good to call this notify_received_message (or something similar) rather than just notify. notify, on its own, is too generic, IMO -- notify of what?

myronmarston · 2014-04-21T15:37:58Z

So here's the part I'm struggling with: playback! creates an expectation on the object which can be fulfilled, which we can then simply call to satisfy. In the case where we're expecting with some args, we need to fulfil that expectation somehow. I don't think any of the notify message received messages accept arguments. Correct me if I'm wrong.

Do you have a spec that demonstrates this case? That would be helpful to use as we figure out a solution. Anyhow, I definitely thinking of expectation args and all that. It sounds like none of the existing methods available off of the any instance recorder will work properly to notify of the exact message expectation (with args) w/o affecting how the object responds to the message (which playback does). So I think you need to refactor the internals of the any instance recorder to expose a method like you need.

myronmarston · 2014-04-25T15:30:06Z

I'm going to take a look at this locally, and maybe push some commits to wrap this up.

fables-tales · 2014-04-25T15:31:52Z

Please do! Sorry I haven't had time to close this out. Let me know if you
need any help/explanations of anything :)

…ance recorders.

myronmarston · 2014-04-30T07:43:14Z

OK, I made some progress and force pushed. More to come. I'm going to bed for now.

myronmarston · 2014-04-30T16:44:42Z

OK, @samphippen, I think this is ready for review and merge.

/cc @JonRowe @xaviershay

fables-tales · 2014-04-30T16:51:33Z

lib/rspec/mocks/any_instance/recorder.rb

@@ -121,6 +121,21 @@ def already_observing?(method_name)
          @observed_methods.include?(method_name) || super_class_observing?(method_name)
        end

+        # @private
+        def notify_received_message(object, message, args, blk)


blk is unused here. Should it be routed somewhere else?

There's nothing to do with blk here (nor with object). They are passed to this method just to provide the full details about the notified message because from the point of view of the proxy calling this, it doesn't know or care which aspects of the received message the subscriber uses.

fables-tales · 2014-04-30T16:54:40Z

I left a few comments. Mostly LGTM. Also travis failed, which I'm not sure about. I'd like to give this another pass once the comments are worked through, but this is great!

…eated previously.

This aligns with the normal `any_instance` behavior.

myronmarston · 2014-04-30T16:57:37Z

Do you still want to move name into the options hash? I think that'd be good.

Longer term, maybe but in the short term now. My immediate concern driving that suggestion was the explicit passing of an unnamed nil arg, but that's no longer necessary and I've removed it, so that immediate concern no longer is a concern.

fables-tales · 2014-04-30T17:01:52Z

OK. This LGTM. I think merge when green.

myronmarston · 2014-04-30T19:09:06Z

So there're two issues with the build:

Travis is having problems with 2.1.0: Ruby 2.1.0 problems travis-ci/travis-ci#2220
1.8.7 is failing when requiring diff/lcs. I think it's a problem with bundler 1.6. This didn't happen on recent builds where bundler 1.5.3 was installed.

xaviershay · 2014-05-01T02:21:40Z

.travis.yml

@@ -1,7 +1,13 @@
 # This file was generated on 2014-03-30T13:16:22-07:00 from the rspec-dev repo.
 # DO NOT modify it by hand as your changes will get lost the next time it is generated.

-before_install: "script/clone_all_rspec_repos"
+before_install:


comment above says not to change this file and I don't see an obvious commit bumping it. Is cool?

myronmarston · 2014-05-01T02:29:42Z

@xaviershay -- the build is failing on 1.8.7 when rspec-expectations tries to require diff/lcs (it gets a aLoadError`). I think it's a regression in bundler 1.6. These code you commented on is troubleshooting code because at the time I couldn't repro the failure locally, so I was pushing troubleshooting code to travis to see the output. I plan to undo all of it before merging this.

xaviershay · 2014-05-01T02:30:48Z

seems legit

JonRowe · 2014-05-01T03:42:15Z

Apart from the travis build issues, this LGTM.

Subscribe recorders to proxies

myronmarston · 2014-05-01T04:43:01Z

Merged, since I got the build to pass finally. I've also got PRs out to rspec-dev, core, expectations, mocks and support to update the .travis.yml accordingly.

JonRowe reviewed Apr 7, 2014
View reviewed changes

myronmarston reviewed Apr 7, 2014
View reviewed changes

Sam Phippen and others added 2 commits April 30, 2014 00:18

Make proxies notify any instance recorders when they receive messages.

7acbc99

Ensure we take the class hierarchy into account when finding any_inst…

a8e1e65

…ance recorders.

myronmarston mentioned this pull request Apr 30, 2014

Any instance expectations cannot be satisfied on instances that have already received calls stubbed messages #652

Closed

fables-tales reviewed Apr 30, 2014
View reviewed changes

myronmarston added 4 commits April 30, 2014 09:55

Ensure our any_instance notification works when the mock proxy was cr…

9cafc85

…eated previously.

Only fulfill any_instance expectations when args match.

30a1751

Don't allow a second instance to receive the message.

3b02aa6

This aligns with the normal `any_instance` behavior.

Add changelog entry.

641bf87

xaviershay reviewed May 1, 2014
View reviewed changes

Work around a few travis/bundler issues.

93b7b10

myronmarston added a commit that referenced this pull request May 1, 2014

Merge pull request #654 from rspec/subscribe-recorders-to-proxies

6d5b911

Subscribe recorders to proxies

myronmarston merged commit 6d5b911 into master May 1, 2014

myronmarston deleted the subscribe-recorders-to-proxies branch May 1, 2014 04:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscribe recorders to proxies #654

Subscribe recorders to proxies #654

fables-tales commented Apr 6, 2014

myronmarston commented Apr 6, 2014

fables-tales commented Apr 6, 2014

JonRowe Apr 7, 2014

fables-tales Apr 7, 2014

JonRowe commented Apr 7, 2014

fables-tales commented Apr 7, 2014

fables-tales commented Apr 7, 2014

myronmarston commented Apr 7, 2014

myronmarston Apr 7, 2014

myronmarston commented Apr 21, 2014

myronmarston commented Apr 25, 2014

fables-tales commented Apr 25, 2014

myronmarston commented Apr 30, 2014

myronmarston commented Apr 30, 2014

fables-tales Apr 30, 2014

myronmarston Apr 30, 2014

fables-tales Apr 30, 2014

fables-tales commented Apr 30, 2014

myronmarston commented Apr 30, 2014

fables-tales commented Apr 30, 2014

myronmarston commented Apr 30, 2014

xaviershay May 1, 2014

myronmarston commented May 1, 2014

xaviershay commented May 1, 2014

JonRowe commented May 1, 2014

myronmarston commented May 1, 2014

Subscribe recorders to proxies #654

Subscribe recorders to proxies #654

Conversation

fables-tales commented Apr 6, 2014

myronmarston commented Apr 6, 2014

fables-tales commented Apr 6, 2014

JonRowe Apr 7, 2014

Choose a reason for hiding this comment

fables-tales Apr 7, 2014

Choose a reason for hiding this comment

JonRowe commented Apr 7, 2014

fables-tales commented Apr 7, 2014

fables-tales commented Apr 7, 2014

myronmarston commented Apr 7, 2014

myronmarston Apr 7, 2014

Choose a reason for hiding this comment

myronmarston commented Apr 21, 2014

myronmarston commented Apr 25, 2014

fables-tales commented Apr 25, 2014

myronmarston commented Apr 30, 2014

myronmarston commented Apr 30, 2014

fables-tales Apr 30, 2014

Choose a reason for hiding this comment

myronmarston Apr 30, 2014

Choose a reason for hiding this comment

fables-tales Apr 30, 2014

Choose a reason for hiding this comment

fables-tales commented Apr 30, 2014

myronmarston commented Apr 30, 2014

fables-tales commented Apr 30, 2014

myronmarston commented Apr 30, 2014

xaviershay May 1, 2014

Choose a reason for hiding this comment

myronmarston commented May 1, 2014

xaviershay commented May 1, 2014

JonRowe commented May 1, 2014

myronmarston commented May 1, 2014