erikdoe / ocmock

Mock objects for Objective-C
http://ocmock.org
Apache License 2.0
2.16k stars 606 forks source link

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

Open dmaclach opened 4 years ago

dmaclach commented 4 years ago

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 commented 4 years ago

I think we could link this to issue #208

dmaclach commented 4 years ago

On Thu, Mar 26, 2020 at 10:43 AM imhuntingwabbits notifications@github.com wrote:

@imhuntingwabbits commented on this pull request.

In Source/OCMock/OCMockObject.m https://github.com/erikdoe/ocmock/pull/394#discussion_r398767673:

  • }
  • } +}
  • +#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 commented 4 years ago

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 commented 4 years ago

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.