Automatically stop all live mocks at the end of each test case

[dmaclach]

If the user is using XCTest with OCMock, this registers a test observer that takes care
of stopping all live mocks appropriately.

For mocks that are created in +setUp, those will get stopped at the end of the suite.
For mocks that are created in -setUp or in test cases themselves, those will get
stopped at the end of the testcase.

While these mocks are being stopped and testcases/suites are being torndown, messages
sent to mocks are not going to trigger the exception about calling a mock after it has
had stopMocking called on it. This allows objects that may refer to mocks in dealloc
methods to be cleaned up in autoreleasepools or due to stopMocking being called
without the mocks throwing exceptions.

This should greatly simplify cleaning up mocks and remove a lot of potential leakage.
It also makes sure that class mocks that mock class methods will not persist across tests.

[dmaclach]

I think we could link this to issue #208

[imhuntingwabbits]

This needs to be customizable. It's not unusual for clients with high concurrency to clean up mocks after the tests have finished.

[dmaclach]

On Thu, Mar 26, 2020 at 10:43 AM imhuntingwabbits ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Source/OCMock/OCMockObject.m <#394 (comment)>: > + } + } +} + +#pragma mark Mock cleanup recording + ++ (void)recordAMockToStop:(OCMockObject *)mock { + [gCurrentMocksToStopRecorder addObject:mock]; +} + ++ (void)removeAMockToStop:(OCMockObject *)mock { + [gCurrentMocksToStopRecorder removeObject:mock]; +} + ++ (void)stopAllTestCaseMocks { + for (OCMockObject *mock in gTestCaseMocksToStop) This needs to be customizable. It's not unusual for clients with high concurrency to clean up mocks after the tests have finished.

Can you expand on what you mean with regards to customizable? Given that this is a NSHashTable with weak pointer mapping, and it runs after the tearDown methods are called should deal with most concerns. Can you give me a concrete use case that you've seen where this wouldn't work?

[imhuntingwabbits]

The minimum bar is disabled, the nice to have bar would be I can invoke some method that opts instances out of this behavior.

Not all mocks are main thread (as xctest defines it) bound, mocks can (and in some cases are) validated off the main queue out of band with the tests.

You can't force those use cases in to the paradigm at the suite level or the test case level. Not everyone cares about XCTests run loop and some use cases register custom observers of their own to allow the threads to coalesce.

[erikdoe]

To be honest, I am not in favour of adding such invasive new behaviour to OCMock at this stage, especially not as on-by-default. While I have no data to back this up, I assume that most codebases using OCMock these days are several years old and have grown over time. Making the change suggested in this PR has the potential of causing any number of hard to diagnose issues in existing test stuites, as @imhuntingwabbits mentions.

I have kept #208 open as an enhancement, because I see how having a list of all mocks can help developers diagnose issues with their tests, discovering release cycles or a lingering mock for class methods for example. I can even see how a stopAllMocks method could be used, even though that would be trivial to implement if OCMock maintained a list of mocks, and even though it might be preferrable to fix the underlying issue rather than just stopping the mocks. The cases where you manually need to stop a mock should be rare.

If you were to change this PR to simply maintain a list of mocks, please double-check the coding conventions, e.g. no prefixing of global variables.

[erikdoe]

This highlights an implicit extension of the contract between the base mock object class and its subclasses. The subclasses need to know that the base class registers the instance "somewhere", so each and every subclass is now required to do special cleanup in case there's an exception in their own init functionality.

We can do this, even if it's inelegant, for the known subclasses in OCMock itself, but it is a breaking change for codebases that have their own subclasses of OCMockObjects, right?

[dmaclach]

I agree that it's inelegant, but the only reason that is there is because of the tests for OCMock itself that test exceptions thrown in Mock objects. The standard Cocoa convention is that exceptions signal programmer error and are not intended to be recovered from, and I would argue that goes doubly for throwing from an init method. By default in Objective C, ARC is not exception-safe; I know this is not the case for Objective C++, but I willing to wager that the amount of folks who are throwing in init from an OCMockObject subclass and expect tests to continue cleanly is vanishingly small (present company excluded of course :) ).

FWIW I'm looking at an extremely large codebase with several hundred iOS/macOS targets with over 45K reference to OCMock in on form or another we have only one subclass of an OCMockObject in any form that I can find (and it would not be affected by this change).

[erikdoe]

As general feedback, why have all these as static variables in this file? Wouldn't it be possible to move the state and logic to a separate class? It could have a singleton (shared instance) to manage the state, right?

[dmaclach]

Will do if that's preferred.

[erikdoe]

The need for this is definitely a smell. I understand the problem, but I'm worried about the complexity that can arise from this additional logic, like "raise when the mock was stopped, but don't raise when it was stopped in a certain way". This'll make diagnosing issues (and answering questions on StackOverflow!) even harder.

[erikdoe]

Is this just an unrelated clean-up? I'm asking because I know about initialize semantics and thought about adding that if statement earlier myself. I decided against it though, because in the end it doesn't add any value I could think of. If the method isn't there you get the exception. If it's there you're potentially testing it more than once, right?

[dmaclach]

Originally the code in the +load was located here and I didn't want it getting initialized more than once. I usually invert your idea and always put an if in my +initialize because I'm afraid that other developers may not understand the initialize semantics and assume it will be called only once. I'll remove as it adds nothing to this particular CL.

[erikdoe]

If the state and logic would live in its own class, it would be possible to make the code more explicit, e.g. that class could have an explicit flag signalling which collection to use and then have an if statement to put the added mock to the right collection.

[erikdoe]

I see what you're doing with these tests, but you these tests only demonstrate that the cleanup doesn't happen under incorrect circumstances. There's no test to show that the cleanup actually happens. Or, in other words, these tests would pass without the functionality added.