Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dmaclach
Copy link
Contributor

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

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

- (void)testThrowsForNilArgumentToUnretainedObjects
{
XCTAssertThrows([OCMArg unretainedObject:nil]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, smaller separate PRs are appreciated. ;-)

@imhuntingwabbits
Copy link
Contributor

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

@dmaclach
Copy link
Contributor Author

As always thanks for looking at these Erik.

erikdoe added a commit that referenced this pull request May 24, 2020
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
Copy link
Contributor Author

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

@erikdoe
Copy link
Owner

erikdoe commented Jun 4, 2020

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.

// 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;
Copy link
Owner

@erikdoe erikdoe Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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;
Copy link
Owner

@erikdoe erikdoe Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gone in the new version

Comment on lines 71 to 91
// 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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gone

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

@erikdoe erikdoe Jun 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

- (NSIndexSet *)unretainedArgumentIndexes;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not be needed. See above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone

[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];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone

#import <Foundation/Foundation.h>

// Do not use directly. See methods and comments in OCMArg.h for usage.
@interface OCMUnretainedArgument : NSObject
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone

// 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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone


- (instancetype)initWithObject:(id)anObject safe:(BOOL)safe;
- (id)object;
- (BOOL)isSafe;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gone

@@ -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])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed up here as well I think

@dmaclach
Copy link
Contributor Author

dmaclach commented Jun 27, 2020

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
Copy link
Owner

erikdoe commented Dec 6, 2020

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants