Verifying null objects only respond to defined methods. #421

Merged
merged 2 commits into from Sep 25, 2013

Conversation

Projects
None yet
4 participants
@xaviershay
Owner

xaviershay commented Sep 25, 2013

  • No additional documentation, since this is the least surprising
    behaviour.

Fixes #392.

@xaviershay

This comment has been minimized.

Show comment Hide comment
@xaviershay

xaviershay Sep 25, 2013

Owner

If this is all that's required, then I'm embarrassed to have procrastinated on this for so long...

Owner

xaviershay commented Sep 25, 2013

If this is all that's required, then I'm embarrassed to have procrastinated on this for so long...

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Sep 25, 2013

Coverage Status

Coverage decreased (-0.07%) when pulling d9121a2 on xaviershay:issue-392 into 331b4b3 on rspec:master.

Coverage Status

Coverage decreased (-0.07%) when pulling d9121a2 on xaviershay:issue-392 into 331b4b3 on rspec:master.

lib/rspec/mocks/verifying_proxy.rb
+ # validity of method expectations will have been checked at definition
+ # time.
+ ensure_implemented(message) if null_object?
+ end

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 25, 2013

Owner

To me, verify_method_missing doesn't really read well for what this method does. Instead, what do you think about this?

  • Make ensure_implemented public.
  • Remove verify_method_missing (you won't need it or anything like it once you make ensure_implemented public).
  • In the method_missing definitions for InstanceVerifyingMock and ClassVerifyingMock, make the first line __mock_proxy.ensure_implemented(message) if null_object? rather than __mock_proxy.verify_method_missing(message).

I think that would communicate better. Thoughts?

@myronmarston

myronmarston Sep 25, 2013

Owner

To me, verify_method_missing doesn't really read well for what this method does. Instead, what do you think about this?

  • Make ensure_implemented public.
  • Remove verify_method_missing (you won't need it or anything like it once you make ensure_implemented public).
  • In the method_missing definitions for InstanceVerifyingMock and ClassVerifyingMock, make the first line __mock_proxy.ensure_implemented(message) if null_object? rather than __mock_proxy.verify_method_missing(message).

I think that would communicate better. Thoughts?

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 25, 2013

Owner

On a side note, do you remember how we arrived at the names InstanceVerifyingMock and ClassVerifyingMock rather than instanceVerifyingDouble and ClassVerifyingDouble? Using Double instead of Mock would seem to be more consistent with the API, and given that mocks aren't stubs and mock vs stub is really a property of how an individual message is treated rather than a property of the test double itself, it's probably good not to use Mock in the class name.

@myronmarston

myronmarston Sep 25, 2013

Owner

On a side note, do you remember how we arrived at the names InstanceVerifyingMock and ClassVerifyingMock rather than instanceVerifyingDouble and ClassVerifyingDouble? Using Double instead of Mock would seem to be more consistent with the API, and given that mocks aren't stubs and mock vs stub is really a property of how an individual message is treated rather than a property of the test double itself, it's probably good not to use Mock in the class name.

This comment has been minimized.

Show comment Hide comment
@xaviershay

xaviershay Sep 25, 2013

Owner
  • Ah I didn't join the dots that null_object? is defined on the double as well. Will change.
  • No idea w/r/t mock, you're right it's weird.
@xaviershay

xaviershay Sep 25, 2013

Owner
  • Ah I didn't join the dots that null_object? is defined on the double as well. Will change.
  • No idea w/r/t mock, you're right it's weird.

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 25, 2013

Owner

No idea w/r/t mock, you're right it's weird.

You up for changing it? Otherwise we'll just add a ticket to do that later.

@myronmarston

myronmarston Sep 25, 2013

Owner

No idea w/r/t mock, you're right it's weird.

You up for changing it? Otherwise we'll just add a ticket to do that later.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 25, 2013

Owner

👍 Looks great, @xaviershay. It's a testament to how well you designed/implemented the earlier verification stuff that this fell out of it so simply!

A changelog entry would be great before merging.

Owner

myronmarston commented Sep 25, 2013

👍 Looks great, @xaviershay. It's a testament to how well you designed/implemented the earlier verification stuff that this fell out of it so simply!

A changelog entry would be great before merging.

xaviershay added some commits Sep 25, 2013

Verifying null objects only respond to defined methods.
* No additional documentation, since this is the least surprising
  behaviour.

Fixes #392.
Rename verifying mocks to doubles.
This is consistent with naming elsewhere.
@xaviershay

This comment has been minimized.

Show comment Hide comment
@xaviershay

xaviershay Sep 25, 2013

Owner

@myronmarston PTAL now.

Owner

xaviershay commented Sep 25, 2013

@myronmarston PTAL now.

@coveralls

This comment has been minimized.

Show comment Hide comment
@coveralls

coveralls Sep 25, 2013

Coverage Status

Coverage decreased (-0.09%) when pulling 3f6328c on xaviershay:issue-392 into 331b4b3 on rspec:master.

Coverage Status

Coverage decreased (-0.09%) when pulling 3f6328c on xaviershay:issue-392 into 331b4b3 on rspec:master.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Sep 25, 2013

Owner

👍 Looks great. Will merge when green.

Owner

myronmarston commented Sep 25, 2013

👍 Looks great. Will merge when green.

JonRowe added a commit that referenced this pull request Sep 25, 2013

Merge pull request #421 from xaviershay/issue-392
Verifying null objects only respond to defined methods.

@JonRowe JonRowe merged commit 8e1a653 into rspec:master Sep 25, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment