Skip to content
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

Convert disableMocking and stopMocking to stopAllMocks #143

Closed

Conversation

cerihughes
Copy link
Contributor

@cerihughes cerihughes commented Jan 11, 2017

Hi Jon,

Here's what I was thinking around the stopAllMocks() functionality I talked about in #141

The idea is that OCMockito will keep a reference to any mocks objects it creates, and when stopAllMocks() is called, it will take care of the intricacies of disabling all mocks, then stopping all mocks, thus freeing the test writer from having to understand the different memory management and crash scenarios that can manifest when doing this manually.

As such, I've also removed disableMocking() and stopMocking() from the public API. Testers now just need to make sure they call stopAllMocks() from the tearDown method (or equivalent) of their test.

Changes in behaviour

Before this change, if a test didn't call stopMocking() on a mock, it may or may not have leaked, depending on the way the mock was used. Now, if a test doesn't call stopAllMocks(), it will leak memory, both from the mocks and all of their message arguments. Likewise, before this change, when setting the mock to nil, deallocation may or may not have run. Now, if stopAllMocks() has been called, deallocation is guaranteed to run when the mock is set to nil.

Other behaviour

Verifying mocks after stopAllMocks() has been called is undefined. As I understand it, this will never fail regardless of how the mock was used prior to stopAllMocks(). This is no different from the current verify behaviour after stopMocking() is called on a mock though.

Let me know what you think. Hopefully, this will lead to a simpler and more understandable API (with respect to memory management at least).

Also, just to be extra clear - this is an API-breaking change.

Thanks!

Cer

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 98.544% when pulling 38866f1 on cerihughes:cerihughes/stop-all-mocks into f0ee7fb on jonreid:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 98.692% when pulling 0c09d27 on cerihughes:cerihughes/stop-all-mocks into f0ee7fb on jonreid:master.

@cerihughes
Copy link
Contributor Author

@jonreid Hold off from reviewing this for now... I may have found another case where a crash can still happen on dealloc :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants