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

Adds spying methods to the RSpec Mocks DSL #671

Merged
merged 1 commit into from Jun 10, 2014

Conversation

Projects
None yet
5 participants
@samphippen
Member

samphippen commented May 10, 2014

Specifically adds:

  • spy - responds and spies all methods
  • instance_spy - responds and spies all methods to which a specified
    class responds.
  • object_spy - responds and spies all methods to which a specific
    object responds
  • class_spy - responds and spies all class methods to which a specific
    class responds

I added test coverage for the expected behaviours and added some YARD
docs to the new methods.

@samphippen

This comment has been minimized.

Member

samphippen commented May 10, 2014

Fixes #665

# methods to which instances of that class respond. Can be used for
# spying with `have_received`.
def instance_spy(doubled_class)
instance_double(doubled_class).as_null_object end

This comment has been minimized.

@xaviershay

xaviershay May 10, 2014

Member

new line for end

This comment has been minimized.

@samphippen

samphippen May 10, 2014

Member

I really don't even.

# Constructs a spy double against a specific class. It responds to all
# class methods to which that class responds. Can be used for spying with
# `have_received`.
def class_spy(doubled_class) class_double(doubled_class).as_null_object

This comment has been minimized.

@xaviershay

xaviershay May 10, 2014

Member

spacing here too

This comment has been minimized.

@samphippen

samphippen May 10, 2014

Member

I don't even.

@xaviershay

This comment has been minimized.

Member

xaviershay commented May 10, 2014

Would probably benefit from @param annotations, and calling out what happens if you spy a method that doesn't exist on the instance/class/object. Or just refer people to verifying double documentation.

@xaviershay

This comment has been minimized.

Member

xaviershay commented May 10, 2014

Should update cukes with examples of new DSL.

@xaviershay

This comment has been minimized.

Member

xaviershay commented May 10, 2014

@myronmarston you tagged this as "post-3.0" feature. Do you feel strongly about merging it beforehand?

It does increase our featureset that we need to guide through the RC, but then it is a fairly straightforward change.

@xaviershay

This comment has been minimized.

Member

xaviershay commented May 10, 2014

Changelog too.

end
it "takes a name" do
expect(spy(:bacon_bits).inspect).to include("bacon_bits")

This comment has been minimized.

@xaviershay

xaviershay May 10, 2014

Member

vegan veto :P

This comment has been minimized.

@JonRowe

JonRowe May 12, 2014

Member

No bacon diet no bacon powers ;)

# for spying with `have_received`
#
# @return (Double)
def spy(name=nil)

This comment has been minimized.

@myronmarston

myronmarston May 10, 2014

Member

Any reason you only support the name argument and not other arguments? I don't see a reason to not support the other double arguments (such as method/return value pairs as a hash). This suggestion applies to the other spy methods as well.

@@ -84,6 +84,34 @@ def object_double(object_or_name, *args)
ExampleMethods.declare_verifying_double(ObjectVerifyingDouble, ref, *args)
end
# Constructs a test double that responds to all methods that can be used
# for spying with `have_received`

This comment has been minimized.

@myronmarston

myronmarston May 10, 2014

Member

"can be used for spying with have_received" is true of all doubles. I think this can be improved a bit:

# Constructs a test double that is optimized for use with `have_received`. With a normal
# double you have to stub a method in order to spy on it, but this automatically spies on
# all methods.

Thoughts?

describe "instance_spy" do
context "when passing a class object" do
let(:the_class) {

This comment has been minimized.

@myronmarston

myronmarston May 10, 2014

Member

Not a merge blocker but my preference is to use do/end for multiline blocks.

This comment has been minimized.

@samphippen

samphippen May 10, 2014

Member

That's fine, I tend to prefer them for semantics, {} for when one is interested in the return value, do...end for when one's caring about some mutation that happens.

This comment has been minimized.

@myronmarston

myronmarston May 12, 2014

Member

I'm aware of that convention but I prefer the do/end for multiliners, curlies for singleliners convention. Would be good to be consistent :).

This comment has been minimized.

@JonRowe

JonRowe May 12, 2014

Member

I also heavily favour do/end for multiliners, curlies for singleliners.

This comment has been minimized.

@cupakromer

cupakromer May 13, 2014

Member

I'm with Sam on this one. Though it's a large code base, so I'd prefer to be consistent.

This comment has been minimized.

@JonRowe

JonRowe May 13, 2014

Member

We also have an existing convention of using do/end blocks for multilines, so this was jarring when reading it. YMMV.

This comment has been minimized.

@samphippen

samphippen May 14, 2014

Member

Gonna fix this :)

@myronmarston

This comment has been minimized.

Member

myronmarston commented May 10, 2014

Should update cukes with examples of new DSL.

Yep, the have_received cukes should show using one of the spy methods for its primary recommended usage.

Also, the YARD docs for have_received should be updated -- currently it uses double('invitation', accept: true) and says "the method must have previously been stubbed in order for messages to be verified", which is inaccurate. Likewise, our README has similar inaccurate language that should be updated.

There's also some errors we raise that have inaccurate wording we should correct:

def raise_expectation_on_unstubbed_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has not been stubbed."
end
# @private
def raise_expectation_on_mocked_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has been mocked instead of stubbed."
end

@myronmarston you tagged this as "post-3.0" feature. Do you feel strongly about merging it beforehand?

It does increase our featureset that we need to guide through the RC, but then it is a fairly straightforward change.

I'm not willing to delay the RC release at all for this (and at this point, we're just waiting on rspec/rspec-core#1519) and once the RC has been released this'll have to wait until after 3.0 final and go into 3.1. If it's ready before I cut the RC release I'm happy for it to be merged and included, though.

@samphippen

This comment has been minimized.

Member

samphippen commented May 11, 2014

There's also some errors we raise that have inaccurate wording we should correct:

def raise_expectation_on_unstubbed_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has not been stubbed."
end
# @private
def raise_expectation_on_mocked_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has been mocked instead of stubbed."
end

I can't think of a case where a spy would cause this behaviour to get triggered. Given that it stubs all methods we shouldn't ever see this error right?

@myronmarston

This comment has been minimized.

Member

myronmarston commented May 11, 2014

I meant that the error message should say something like "or use a spy" so that it's not presenting stubbing the method as the only option.

Sent from my iPhone

On May 11, 2014, at 8:03 AM, Sam Phippen notifications@github.com wrote:

There's also some errors we raise that have inaccurate wording we should correct:

def raise_expectation_on_unstubbed_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has not been stubbed."
end
# @private
def raise_expectation_on_mocked_method(method)
__raise "#{intro} expected to have received #{method}, but that " +
"method has been mocked instead of stubbed."
end

I can't think of a case where a spy would cause this behaviour to get triggered. Given that it stubs all methods we shouldn't ever see this error right?


Reply to this email directly or view it on GitHub.

# a normal double one has to stub methods in order to be able to spy
# them. An instance_spy automatically spies on all instance methods to
# which the class responds.
def instance_spy(doubled_class, *args)

This comment has been minimized.

@myronmarston

myronmarston May 12, 2014

Member

Why is doubled_class called out as an arg? Why not use *args for everything?

# are allowed to be stubbed. With a normal double one has to stub
# methods in order to be able to spy them. An object_spy automatically
# spies on all methods to which the object responds.
def object_spy(doubled_instance, *args)

This comment has been minimized.

@myronmarston

myronmarston May 12, 2014

Member

Same question here...

# With a normal double one has to stub methods in order to be able to spy
# them. An class_spy automatically spies on all class methods to which the
# class responds.
def class_spy(doubled_class, *args)

This comment has been minimized.

@myronmarston

myronmarston May 12, 2014

Member

...and here.

README.md Outdated
@@ -103,14 +103,13 @@ zipcode.valid?
## Test Spies
Verifies the given object received the expected message during the course of the
test. The method must have previously been stubbed in order for messages to be
verified.
test.

This comment has been minimized.

@myronmarston

myronmarston May 12, 2014

Member

I think it's still useful to have a sentence explaining the requirements for spying. Maybe something like this:

For a message to be verified, the given object must be setup to spy on it, either by having
it explicitly stubbed or by being defined as a `spy`.
@samphippen

This comment has been minimized.

Member

samphippen commented Jun 6, 2014

@myronmarston is this good to merge now that RSpec 3 is out?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 6, 2014

@myronmarston is this good to merge now that RSpec 3 is out?

Do you mind holding off until I merge #695 (I plan to as soon as the build goes green)? I want to merge that branch to both 3-0-maintenance and master branches but the changes in this PR can't go in 3-0-maintenance so it's easiest to hold off until I merge if you're fine with that. We'll also need to update the cukes (based on what's in #695) to use these APIs rather than as_null_object for creating spies.

@samphippen

This comment has been minimized.

Member

samphippen commented Jun 6, 2014

@myronmarston I rebased this on master post #695. I modified the features that point at spies too. Can you take a look and tell me what you think?

README.md Outdated
verified.
Verifies the given object received the expected message during the course of
the test. For a message to be verified, the given object must be setup to spy
on it, either by having it explicitly stubbed or by being defined as a `spy`.

This comment has been minimized.

@myronmarston

myronmarston Jun 6, 2014

Member

Relish is very picky about line breaks in rendering. Have you pushed this to the staging environment to confirm it looks good? (Run rake relish_staging to push it).

This comment has been minimized.

@myronmarston

myronmarston Jun 6, 2014

Member

Actually, nevermind about that -- I just realized this is the README, which isn't published to relish. The suggestion applies when you update the narrative of spies.feature, though.

I also think it'd be good to for this to clarify what a spy is, and to mention that there are instance_spy, class_spy and object_spy versions. Maybe something like this?

Verifies the given object received the expected message during the course of
the test. For a message to be verified, the given object must be setup to spy
on it, either by having it explicitly stubbed or by being a null object double
(e.g. `double(...).as_null_object`). Convenience methods are provided to easily
null object doubles for this purpose:

spy("invitation") # => same as `double("invitiation").as_null_object`
instance_spy("Invitation") # => same as `instance_double("Invitiation").as_null_object`
class_spy("Invitation") # => same as `class_double("Invitiation").as_null_object`
object_spy("Invitation") # => same as `object_double("Invitiation").as_null_object`
@@ -19,7 +19,7 @@ Feature: Spies
"""ruby
RSpec.describe "have_received" do
it "passes when the message has been received" do
invitation = double('invitation').as_null_object
invitation = spy('invitation')

This comment has been minimized.

@myronmarston

myronmarston Jun 6, 2014

Member

This spies.feature file also has a narrative that should be updated.

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 6, 2014

Looking good. Left a few comments about the prose. Would be good to squash this into one commit as well.

@samphippen

This comment has been minimized.

Member

samphippen commented Jun 7, 2014

@myronmarston I don't have relish staging access, I'm not even sure I've got an account. What do I need to do? In the meantime I'm gonna push the commits I've written. I don't have that much experience writing relish docs so any feedback would be appreciated.

README.md Outdated
the test. For a message to be verified, the given object must be setup to spy
on it, either by having it explicitly stubbed or by being a null object double
(e.g. `double(...).as_null_object`). Convenience methods are provided to easily
null object doubles for this purpose:

This comment has been minimized.

@myronmarston

myronmarston Jun 7, 2014

Member

This is missing a word, I think, maybe should be "...to easily create null object doubles..."?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 7, 2014

Create an account on relish and I'll invite you.

@samphippen

This comment has been minimized.

Member

samphippen commented Jun 8, 2014

@myronmarston I now have an account (email is samphippen@googlemail.com). I got this error when trying to push to staging:

Authorization failed: You are not a collaborator on the project 'rspec-staging/rspec-mocks'
@@ -7,25 +7,25 @@ Feature: Spies
`have_received`.
You can use any test double (or partial double) as a spy, but the double must be setup to
spy on the messages you care about. [Null object doubles](./null-object-doubles) automatically spy on all messages,
spy on the messages you care about. [Spies] automatically spy on all messages,

This comment has been minimized.

@myronmarston

myronmarston Jun 9, 2014

Member

Do you mean for [Spies] to be a link?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 9, 2014

I tried adding you as a collaborator but can't figure out how :(. It's super simple to push to your own relish account, though -- maybe that's what you can use next time. This time I pushed and thought it looked OK except for the oddness of "Spies" having square brackets around it.

@JonRowe

This comment has been minimized.

Member

JonRowe commented Jun 9, 2014

I love how this is mostly docs :)

@samphippen

This comment has been minimized.

Member

samphippen commented Jun 9, 2014

@myronmarston shall I shred those square brackets, then squash and merge?

@myronmarston

This comment has been minimized.

Member

myronmarston commented Jun 9, 2014

@myronmarston shall I shred those square brackets, then squash and merge?

👍

@JonRowe

This comment has been minimized.

Member

JonRowe commented Jun 9, 2014

:shipit:

@samphippen

This comment has been minimized.

Member

samphippen commented Jun 10, 2014

I added a changelog entry, I'm going to merge this when it goes green.

Enhancements:
* Add spyinng methods (`spy`, `ìnstance_spy`, `class_spy` and `object_spy`)

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014

Member

This has an extra n.

* Add spyinng methods (`spy`, `ìnstance_spy`, `class_spy` and `object_spy`)
which create doubles as null objects for use with spying in testing. (Sam
Phippen #671)

This comment has been minimized.

@myronmarston

myronmarston Jun 10, 2014

Member

Should have a comma between your name and the issue #.

Adds spying methods to the RSpec Mocks DSL
Specifically adds:

* spy - responds and spies all methods
* instance_spy - responds and spies all methods to which a specified
  class responds.
* object_spy - responds and spies all methods to which a specific
  instance responds
* class_spy - responds and spies all class methods to which a specific
  class responds

I added test coverage for the expected behaviours and added some YARD
docs to the new methods.

samphippen added a commit that referenced this pull request Jun 10, 2014

Merge pull request #671 from rspec/spy-methods
Adds spying methods to the RSpec Mocks DSL

@samphippen samphippen merged commit b53f2be into master Jun 10, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@samphippen samphippen deleted the spy-methods branch Jun 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment