search

erikdoe / ocmock

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

OCMockObject invocations retain arguments #360

Closed ian-twilightcoder closed 4 years ago

ian-twilightcoder commented 6 years ago

OCMockObject invocations need to retain their arguments, however that can introduce retain cycles that wouldn't otherwise be there. It also makes dealloc tests fail for objects that get sent as arguments to mocks. Break the chain in -stopMocking by emptying out the invocations.

https://github.com/erikdoe/ocmock/issues/307

ian-twilightcoder commented 6 years ago

This is based on the suggestion in the issue by @fareal that -stopMocking be used to release arguments to mock objects, and potentially break retain cycles that can result as described by @alexito4.

The gotcha here is that mocks can no longer be verified after -stopMocking has been called. In practice I don't see how that could be an issue, but it is a behavior change that I want to point out up front.

I don't see any great alternatives though.

  1. Add a way to opt-out of the autorelease-safe and thread-safe behavior introduced in #171 a la #348. For people who really know what they're doing and can guarantee that nothing in the invocation will get released for the lifetime of the mock, then ok, but that seems sketchy.
  2. Take @fareal's solution verbatim. The problem with his solution is that -stopMocking would then restore the original problem where the invocations can hold zombies, which can cause crashes. If you don't use the mock after -stopMocking (and it seems to me like you really shouldn't) then it's not a problem. But if you do keep using the mock, probably to verify, it seems better to me to fail the verifies rather than sporadically crash.
  3. Add an explicit method to empty out all of the captured invocations. This would allow verification after -stopMocking, but I don't know if anyone actually wants to do that so I don't know if it's really worth adding another method.
  4. Do nothing. I think the retain cycle problem at least is worth fixing. And even though I'm not a big fan, dealloc tests are a thing that people do in real life. There are several related issues/complaints here that would seem to prove that. So I think it's worth making those possible too.

So I think the behavior change is worth it on principle, and I think it will make more people happy than it makes angry?

imhuntingwabbits commented 6 years ago

I think this proposal is fine on the face of it. -stopMocking IMO is essentially -tearDown for the mock. But using it afterwards should fail aggressively with appropriate warnings / messages.

Would you consider amending your diff to add a fault, maybe NSException, or if we want to be really aggressive a hard coded call to abort() if -expect is called on a mock after -stopMocking?

Also if I read this right -stopMocking is now a required call for all types of mocks, not just class or partials?

ian-twilightcoder commented 6 years ago

No, this proposal doesn't make -stopMocking a required call for any mocks where it wasn't already. It just makes it so that -stopMocking can be used to manually release the captured invocations on the mock, rather than relying on the mock's own object lifecycle. This is particularly important when the mock retains objects that the real object normally wouldn't, and the objects it's retaining in turn hold a strong reference to the mock. Prior to this proposal there was no way out of that situation, but with this proposal you can now manually call -stopMocking to break the retain cycle. So I wouldn't say this adds a requirement, but rather it adds an option to fix a previously un-fixable situation.

I'm not opposed to installing a land mine after -stopMocking is called, but I think that should be a separate diff/proposal.

ian-twilightcoder commented 5 years ago

Hi @erikdoe what do you think of this one? I still think it's the best compromise between all the proposed solutions, code snippets, and pull requests for the issue. But I'm also happy to make changes as you prefer.

matrush commented 5 years ago

@erikdoe Any updates on this one? It's kind annoying that the arguments were captured after stopMocking is called 😢

erikdoe commented 4 years ago

It certainly took a very long time for me to merge this. Somehow, against the odds, I was still hoping to find a more elegant solution. I didn't.

After merging this PR I also added the "land mines" mentioned above. If tests try to use the verify after run functionality or the mock itself, an exception will be thrown. I thought this was super important because without it, the exception thrown when trying to verify after run after stopMocking would have been very misleading.

ian-twilightcoder commented 4 years ago

Woohoo! Thank you for checking this one out, and also doing the follow up land mines too!