Add support for unretained and unsafeunretained arguments for stubs. #419

dmaclach · 2020-05-21T21:05:13Z

Allows marking an argument in a stub as being either 'unsafe' or 'unsafeunretained'.
An unsafe object argument is retained by the stub, but not by invocations on the mock.
An unsafe unretained object argument is not retained by the stub or by invocations on the
mock.

This allows for mocking of methods that do not retain their arguments. This should
simplify testing of methods that are called in dealloc, and give people a way out of
other retain-loop problems.

Fix for #347 , #346 and may help with some of the discussion on #398

imhuntingwabbits · 2020-05-21T22:45:55Z

Source/OCMockTests/OCMArgTests.m

@@ -101,4 +101,10 @@ - (void)testHandlesNonObjectPointersGracefully
    XCTAssertEqual([OCMArg resolveSpecialValues:nonObjectPointerValue], nonObjectPointerValue, @"Should have returned value as is.");
 }

+- (void)testThrowsForNilArgumentToUnretainedObjects
+{
+    XCTAssertThrows([OCMArg unretainedObject:nil]);


This should verify that the message is correct. You may wish to invent a macro like this one to make that easier:

#define XCTAssertThrowsEquivalent(EXPECTED, THROWING_CALL) \ ({ \ BOOL threw = NO;\ NSException *capturedException = nil; \ @try { \ THROWING_CALL; \ } @catch (NSException *thrownException) { \ capturedException = [thrownException retain]; \ threw = YES; \ } @finally { \ XCTAssertTrue(threw, @"Expected exception was not thrown: %@", EXPECTED); \ if (threw) {\ if ([[capturedException name] isEqualToString:@"OCMStubbedException"]) { \ id actualException = [[[capturedException userInfo] objectForKey:@"exception"] retain]; \ XCTAssertNotNil(actualException, @"Exception thrown by OCMock doesn't include the expected underlying exception: %@\n%@", EXPECTED, capturedException); \ XCTAssertExceptionMatchesException(EXPECTED, actualException); \ [actualException release]; actualException = nil; \ } else { \ XCTAssertExceptionMatchesException(EXPECTED, capturedException); \ } \ } \ [capturedException release]; capturedException = nil; \ } \ })

That depends on a plethora of other macros:

#define XCTAssertExceptionMatchesException(EXPECTED, ACTUAL) \ ({ \ XCTAssertStringMatchesString([EXPECTED name], [ACTUAL name]); \ XCTAssertStringMatchesString([(NSException *)EXPECTED reason], [(NSException *)ACTUAL reason]); \ XCTAssertDictionaryMatchesDictionary([EXPECTED userInfo], [ACTUAL userInfo]); \ }) #define XCTAssertStringMatchesString(EXPECTED, ACTUAL) \ ({ \ if (EXPECTED == nil) { \ if (ACTUAL != nil) { \ XCTFail(@"String mismatch:\n%@\n%@", [XCTestingAsserts trimExcessiveValuesForLog:EXPECTED], [XCTestingAsserts trimExcessiveValuesForLog:ACTUAL]); \ } \ } else if (ACTUAL == nil) { \ XCTFail(@"String mismatch:\n%@\n%@", [XCTestingAsserts trimExcessiveValuesForLog:EXPECTED], [XCTestingAsserts trimExcessiveValuesForLog:ACTUAL]); \ } else { \ XCTAssertTrue([EXPECTED isEqualToString:ACTUAL], @"String mismatch:\n%@\n%@", [XCTestingAsserts trimExcessiveValuesForLog:EXPECTED], [XCTestingAsserts trimExcessiveValuesForLog:ACTUAL]); \ } \ }) #define XCTAssertDictionaryMatchesDictionary(EXPECTED, ACTUAL) \ ({ \ if (EXPECTED != ACTUAL) { \ if ((EXPECTED == nil) || (ACTUAL == nil)) { \ XCTFail(@"Dictionary mismatch: %@ / %@", EXPECTED, ACTUAL); \ } else { \ XCTAssertTrue([EXPECTED isEqualToDictionary:ACTUAL], @"Dictionary mismatch: %@ / %@", EXPECTED, ACTUAL); \ } \ } \ })

Sorry, I know there's a lot here, but it really does make writing complete tests easier.

I'm guessing that this should probably come in a separate PR and get applied to all of the throw checks. Thoughts Erik?

Yes, smaller separate PRs are appreciated. ;-)

imhuntingwabbits · 2020-05-21T22:47:54Z

Excellent. I haven't done a very thorough pass but this looks exactly like the evolution I was asking for in #398.

Source/OCMock/NSInvocation+OCMAdditions.m

Source/OCMock/OCMInvocationMatcher.m

dmaclach · 2020-05-23T21:03:07Z

As always thanks for looking at these Erik.

At this stage I am not sure whether to merge a few more (#419, #404, #398) and release as 3.6.1 or whether to include #421 (and maybe a variation on #394) and then release as 3.7.

dmaclach · 2020-05-28T15:58:37Z

Found an issue where calling "isKindOfClass" on proxies was causing issues. Replaced with [OCMArg isUnretained] which calls objc_getClass instead.

erikdoe · 2020-06-04T21:02:07Z

So, I looked at this again in detail. I get the intent, but I don't understand the implementation. It's easier to comment directly in the code, which is what I'll do below.

I'll also comment on a few other points.

erikdoe · 2020-06-04T21:04:41Z

Source/OCMock/OCMArg.h

+// stub itself. A use case for this is when you are stubbing an argument to a method that does not
+// retain its argument using an `OCMArg` variant that you do not want to keep a reference to.
+// See `OCMOCK_ANY_UNRETAINED`.
+ (id)unretainedObject:(id)anObject;


This make sense. I'm just wondering whether we can find a better name. I mean, it's not that the argument is an object that is unretained, or that the argument has to be an unretained object, it's more like the argument is an object that is not to be retained by the mock, right?

The comment in the code confused me. I can clarify the first part, but I'm not sure what to do with the part about the use case. I guess, I don't understand how you stub an argument. I think you stub a method and the argument is a constraint that indicates when the stub applies, not?

OK.. so based on your feedback and some thoughts that I've had over the past couple of weeks I spent a good chunk of the day doing a pretty considerable rewrite. I attempted to break it up into a bunch of individual commits as best I could. It gets rid of the OCMUnretainedArgument class and adds a collection of options to all of the OCMArg functions for controlling memory management at stub and invocation time. It makes it very convenient to add in the copy semantic that we were missing previously. I think it should help with the naming issues that concerned you. It's definitely a little more invasive though 😄 .

See what you think. I think the options also make it very easy over a large codebase to find these "special" cases.

erikdoe · 2020-06-04T21:06:54Z

Source/OCMock/OCMArg.h

@@ -45,10 +61,18 @@

 + (id)resolveSpecialValues:(NSValue *)value;

+// Return YES if `object` is either an unretained or an unsafe unretained object.
+ (BOOL)isUnretained:(id)object;


Your comment on the PR explains the need for this method, but why is this on OCMArg? Wouldn't it make more sense for it to be on OCMUnretainedArgument? And if so, is there a better name for this method? I mean, it clearly doesn't return true when the argument is somehow retained, right?

This is gone in the new version

erikdoe · 2020-06-04T21:08:05Z

Source/OCMock/OCMArg.h

+// See comments on [OCMArg unretainedObject] and [OCMArg unsafeUnretainedObject].
+#define OCMOCK_UNSAFE_UNRETAINED(x) [OCMArg unsafeUnretainedObject:(x)]
+#define OCMOCK_UNRETAINED(x) [OCMArg unretainedObject:(x)]
+#define OCMOCK_ANY_UNRETAINED OCMOCK_UNRETAINED(OCMOCK_ANY)


Please do not add these macros. I only kept OCMOCK_ANY around for backwards compatibility. IIRC it predates OCMArg.

erikdoe · 2020-06-04T21:14:53Z

Source/OCMock/OCMInvocationMatcher.h

@@ -21,6 +21,7 @@
    NSInvocation *recordedInvocation;
    BOOL         recordedAsClassMethod;
    BOOL         ignoreNonObjectArgs;
+    NSIndexSet   *unretainedArgumentIndexes;


This is what I don't understand. Do we really have to remember the arguments' indices as state in the matcher? Wouldn't it be equally possible to keep the OCMUnretainedArgument wrappers around the args in the invocation and test for that in matchesInvocation?

Such an approach would be more in line with the current implementation. We keep the special values on the recorded invocation and test for them in matchesInvocation.

That said, I get that this case is somewhat different because we need the information, too, when the mock records all the invocations it sees.

So the new version is more in line with the current implementation. The logic for managing stub arguments is kept in the OCMConstraints, and the logic for managing invocation arguments is in the invocation.

erikdoe · 2020-06-04T21:15:24Z

Source/OCMock/OCMInvocationMatcher.h

@@ -34,4 +35,6 @@
 - (BOOL)matchesSelector:(SEL)aSelector;
 - (BOOL)matchesInvocation:(NSInvocation *)anInvocation;

+- (NSIndexSet *)unretainedArgumentIndexes;


This would not be needed. See above.

erikdoe · 2020-06-04T21:22:34Z

Source/OCMock/OCMInvocationMatcher.m

    [recordedInvocation release];
    // Don't do a regular -retainArguments on the invocation that we use for matching. NSInvocation
    // effectively does an strcpy on char* arguments which messes up matching them literally and blows
    // up with anyPointer (in strlen since it's not actually a C string). Also on the off-chance that
    // anInvocation contains self as an argument, -retainArguments would create a retain cycle.
-    [anInvocation retainObjectArgumentsExcludingObject:self];
+    [anInvocation retainObjectArgumentsExcludingObject:self excludingObjectsAtIndexes:unsafeUnretainedIndexes];


This would not be needed. The invocation object would just retain the wrapper, which already has the retain / do-not-retain logic.

erikdoe · 2020-06-04T21:23:23Z

Source/OCMock/OCMUnretainedArgument.h

+#import <Foundation/Foundation.h>
+
+// Do not use directly. See methods and comments in OCMArg.h for usage.
+@interface OCMUnretainedArgument : NSObject


The comment on naming from above applies here to. It's not an argument that is unretained. It's an argument that is not to be retained.

erikdoe · 2020-06-04T21:33:33Z

Source/OCMock/OCMArg.h

+// Note that you *must* keep a reference to anObject outside this call or you will crash.
+// Something like `[OCMArg unsafeUnretainedObject:[[Foo alloc] init]]` under ARC is a guaranteed
+// dangling pointer problem.
+ (id)unsafeUnretainedObject:(id)anObject;


Similar to my comment above -- name and clarification of comment in code.

erikdoe · 2020-06-04T22:07:40Z

Source/OCMock/OCMUnretainedArgument.h

+
+- (instancetype)initWithObject:(id)anObject safe:(BOOL)safe;
+- (id)object;
+- (BOOL)isSafe;


This would not be needed.

erikdoe · 2020-06-04T22:10:02Z

Source/OCMock/NSInvocation+OCMAdditions.m

@@ -80,7 +80,16 @@ - (void)retainObjectArgumentsExcludingObject:(id)objectToExclude
    for(NSUInteger index = 2; index < numberOfArguments; index++)
    {
        const char *argumentType = [[self methodSignature] getArgumentTypeAtIndex:index];
-        if(OCMIsObjectType(argumentType))
+        BOOL isObjectType = OCMIsObjectType(argumentType);
+        if ([indexes containsIndex:index])


Given the proposed implementation in the mock itself, the indexes argument here can be nil. Of course, the if statement still works in that case, but I must say it did confuse me a bit.

Fixed up here as well I think

dmaclach · 2020-06-27T19:34:52Z

I hadn't realized that my last push had put this in a horribly mixed up state. I think it's much cleaner now.

erikdoe · 2020-12-06T15:12:12Z

Apologies but I have moved this change to OCMock 3.9 for now. The 3.8 release has taken much longer than I had hoped and there are a number of good changes in it already, which I'd like to get out soon. This PR still needs some work. The conflicts aren't too bad but there are a number of details (naming and otherwise) which I'd want to address before merging.

Most constraints now require init values, so having `+constraint` is relatively useless.

…ubs. Allows marking an argument in a stub as having various semantics: - is not retained by invocations Object arguments are retained by default in OCMock. In some cases to avoid retain loops you need to mark an argument as unretained. - is not retained by stub Stub arguments are retained by default in OCMock. In some specialized cases you do not want the stub arguments retained - is copied by invocation Some arguments have copy semantics and we need the invocation to copy the argument instead of retain it.

imhuntingwabbits reviewed May 21, 2020

View reviewed changes

erikdoe reviewed May 23, 2020

View reviewed changes

Source/OCMock/NSInvocation+OCMAdditions.m Outdated Show resolved Hide resolved

erikdoe reviewed May 23, 2020

View reviewed changes

Source/OCMock/NSInvocation+OCMAdditions.m Outdated Show resolved Hide resolved

erikdoe reviewed May 23, 2020

View reviewed changes

Source/OCMock/OCMInvocationMatcher.m Outdated Show resolved Hide resolved

erikdoe added the waiting for update label May 23, 2020

dmaclach force-pushed the weakPtrSupport branch from 60fbf49 to 117fe12 Compare May 23, 2020 21:00

erikdoe removed the waiting for update label May 24, 2020

dmaclach force-pushed the weakPtrSupport branch from 117fe12 to c9b8923 Compare May 28, 2020 15:57

erikdoe reviewed Jun 4, 2020

View reviewed changes

erikdoe added the waiting for update label Jun 4, 2020

dmaclach force-pushed the weakPtrSupport branch from a8b4878 to 6c1c386 Compare June 6, 2020 01:53

dmaclach force-pushed the weakPtrSupport branch 3 times, most recently from 0e5d344 to 9d27214 Compare June 18, 2020 23:29

erikdoe removed the waiting for update label Jun 22, 2020

erikdoe added this to the OCMock 3.8 milestone Jun 22, 2020

dmaclach force-pushed the weakPtrSupport branch from 9d27214 to 7156c9e Compare June 27, 2020 19:31

dmaclach force-pushed the weakPtrSupport branch from 7156c9e to 016513d Compare July 14, 2020 20:45

dmaclach force-pushed the weakPtrSupport branch from 016513d to cc8f538 Compare August 1, 2020 00:06

dmaclach force-pushed the weakPtrSupport branch from cc8f538 to 3612a15 Compare August 13, 2020 15:52

erikdoe modified the milestones: OCMock 3.8, OCMock 3.9 (or later) Dec 6, 2020

dmaclach force-pushed the weakPtrSupport branch from 3612a15 to a7a5473 Compare January 2, 2021 18:46

dmaclach added 5 commits February 15, 2021 11:26

Add OCMIsEqualConstraint

0553323

Make OCMInvocationConstraint retain invocation

2b6f0c8

Remove +constraint method

da28d71

Most constraints now require init values, so having `+constraint` is relatively useless.

Set up designated initializer for OCMBlockConstraint

c34a820

dmaclach force-pushed the weakPtrSupport branch from a7a5473 to a22241f Compare February 15, 2021 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for unretained and unsafeunretained arguments for stubs. #419

Add support for unretained and unsafeunretained arguments for stubs. #419

dmaclach commented May 21, 2020

imhuntingwabbits May 21, 2020

dmaclach May 21, 2020

erikdoe May 23, 2020

imhuntingwabbits commented May 21, 2020

dmaclach commented May 23, 2020

dmaclach commented May 28, 2020

erikdoe commented Jun 4, 2020 •

edited

erikdoe Jun 4, 2020 •

edited

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020 •

edited

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020 •

edited

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

erikdoe Jun 4, 2020

dmaclach Jun 6, 2020

dmaclach commented Jun 27, 2020 •

edited

erikdoe commented Dec 6, 2020

Add support for unretained and unsafeunretained arguments for stubs. #419

Are you sure you want to change the base?

Add support for unretained and unsafeunretained arguments for stubs. #419

Conversation

dmaclach commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imhuntingwabbits commented May 21, 2020

dmaclach commented May 23, 2020

dmaclach commented May 28, 2020

erikdoe commented Jun 4, 2020 • edited

erikdoe Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdoe Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdoe Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmaclach commented Jun 27, 2020 • edited

erikdoe commented Dec 6, 2020

erikdoe commented Jun 4, 2020 •

edited

erikdoe Jun 4, 2020 •

edited

erikdoe Jun 4, 2020 •

edited

erikdoe Jun 4, 2020 •

edited

dmaclach commented Jun 27, 2020 •

edited