erikdoe / ocmock

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

Show description when throwing exceptions because of no invocations #381

Closed matrush closed 4 years ago

matrush commented 4 years ago

This resolves #380 Before:

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: '** Cannot handle or verify invocations. This error usually occurs when a mock object is used after stopMocking has been called on it. In most cases it is not necessary to call stopMocking. If you know you have to, please make sure that the mock object is not used afterwards.'

After:

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: '** Cannot handle or verify invocations on OCMockObject(CLLocationManager). This error usually occurs when a mock object is used after stopMocking has been called on it. In most cases it is not necessary to call stopMocking. If you know you have to, please make sure that the mock object is not used afterwards.'
imhuntingwabbits commented 4 years ago

This deserves a test to validate the exception message. A number of tests already trigger this exception:

Test Case '-[OCMockObjectInternalTests testRaisesWhenAttemptingToUseAfterStopMocking]' started.
Test Case '-[OCMockObjectInternalTests testRaisesWhenAttemptingToUseAfterStopMocking]' passed (1.926 seconds).
Test Case '-[OCMockObjectInternalTests testRaisesWhenAttemptingToVerifyInvocationsAfterStopMocking]' started.
Test Case '-[OCMockObjectInternalTests testRaisesWhenAttemptingToVerifyInvocationsAfterStopMocking]' passed (1.684 seconds).
Test Case '-[OCMockObjectVerifyAfterRunTests testThrowsWhenVerificationIsAttemptedAfterStopMocking]' started.
Test Case '-[OCMockObjectVerifyAfterRunTests testThrowsWhenVerificationIsAttemptedAfterStopMocking]' passed (2.046 seconds).

So updating one or more of those tests to validate the message seems like a good way to ensure this doesn't regress.

matrush commented 4 years ago

Will update, thanks!

matrush commented 4 years ago

Updated, maybe good to go now?

imhuntingwabbits commented 4 years ago

IMO yes but I don't own that.

erikdoe commented 4 years ago

To be honest, I'm hesitating to merge this. On the one hand I see the value. On the other hand, hopefully tests are reproducible and it's not hard to set a breakpoint on the exception; something you'll likely want to do anyway when figuring out how the mock is used, right?

matrush commented 4 years ago

@erikdoe I'm requesting this because in my recent migration from OCMock 3.4 to OCMock 3.5, I have seen hundreds of tests failing in a large codebase due to all the new exceptions thrown. It's very hard to go through one by one and figure out which object is not mocked properly. So I added it locally and it indeed saved me a lot of time.

Given this, I feel this brings value to user, and would be good to be merged :)

imhuntingwabbits commented 4 years ago

FWIW I second this. The more context a failure message can give the better. For individual developers debugging inline / reproducing at desk is easy, but for teams with screeners / large CI infrastructures the additional context can help a lot when trying to correlate failures.

matrush commented 4 years ago

Thanks @erikdoe!