`any_instance` partial mocks aren't reset #416

Merged
merged 2 commits into from Sep 21, 2013

Conversation

Projects
None yet
3 participants
@JonRowe
Member

JonRowe commented Sep 10, 2013

As I summised in #399 partial mocks setup via the any_instance recorders aren't
reset by the conventional process, instead relying on their own verify process to
remove all methods setup by recorders. I'm not sure if this is an issue but this
demonstrates what I'm talking about.

Do not merge.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 15, 2013

Member

@myronmarston did you get a chance to review this after merging the receive_messages stuff?

Member

JonRowe commented Sep 15, 2013

@myronmarston did you get a chance to review this after merging the receive_messages stuff?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 15, 2013

Member

Looking at it now for the first time :).

So basically the issue is the inconsistency/slight weirdness of the fact that proxy_for(obj).reset doesn't reset obj if it was stubbed via any_instance?

Seems like in practice, this isn't actually a problem for end users because RSpec resets the any_instance recorders as well, so the stubs get cleared between examples still, right?

I'd say that some simple refactorings that reduce/remove the inconsistencies would be nice but are definitely not a priority given the other things we have to work on and the fact that I can't think of a way for end-users to run into any bugs here.

Member

myronmarston commented Sep 15, 2013

Looking at it now for the first time :).

So basically the issue is the inconsistency/slight weirdness of the fact that proxy_for(obj).reset doesn't reset obj if it was stubbed via any_instance?

Seems like in practice, this isn't actually a problem for end users because RSpec resets the any_instance recorders as well, so the stubs get cleared between examples still, right?

I'd say that some simple refactorings that reduce/remove the inconsistencies would be nice but are definitely not a priority given the other things we have to work on and the fact that I can't think of a way for end-users to run into any bugs here.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 16, 2013

Member

My concern was more that we check that mocks reset cleanly, but not the any instance ones. So far I haven't been able to force the any instance recorders to cleanup on the partial mocks either. (Generally not a problem between tests because instances don't survive across tests).

Member

JonRowe commented Sep 16, 2013

My concern was more that we check that mocks reset cleanly, but not the any instance ones. So far I haven't been able to force the any instance recorders to cleanup on the partial mocks either. (Generally not a problem between tests because instances don't survive across tests).

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 18, 2013

Member

My concern was more that we check that mocks reset cleanly, but not the any instance ones.

Sure we do:

https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/any_instance_spec.rb#L711-L855

So far I haven't been able to force the any instance recorders to cleanup on the partial mocks either. (Generally not a problem between tests because instances don't survive across tests).

It doesn't cleanup with a simple reset partial_mock, but it should if you call RSpec::Mocks.space.verify_all.

Member

myronmarston commented Sep 18, 2013

My concern was more that we check that mocks reset cleanly, but not the any instance ones.

Sure we do:

https://github.com/rspec/rspec-mocks/blob/master/spec/rspec/mocks/any_instance_spec.rb#L711-L855

So far I haven't been able to force the any instance recorders to cleanup on the partial mocks either. (Generally not a problem between tests because instances don't survive across tests).

It doesn't cleanup with a simple reset partial_mock, but it should if you call RSpec::Mocks.space.verify_all.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 19, 2013

Member

Ok I think it's useful to add these tests, so I've fixed them up to pass.

Member

JonRowe commented Sep 19, 2013

Ok I think it's useful to add these tests, so I've fixed them up to pass.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 19, 2013

Coverage Status

Coverage decreased (-0.09%) when pulling 08c6637 on any_instance_resetting into fd78578 on master.

Coverage Status

Coverage decreased (-0.09%) when pulling 08c6637 on any_instance_resetting into fd78578 on master.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 19, 2013

Member

Unless anyone objects these are good to merge...

Member

JonRowe commented Sep 19, 2013

Unless anyone objects these are good to merge...

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

Merge pull request #416 from rspec/any_instance_resetting
`any_instance` partial mocks aren't reset

@JonRowe JonRowe merged commit 331b4b3 into master Sep 21, 2013

1 check passed

default The Travis CI build passed
Details

@JonRowe JonRowe deleted the any_instance_resetting branch Sep 21, 2013

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