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

Fix multiple calls in same recorder #485

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion Source/OCMock/OCMRecorder.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
{
OCMockObject *mockObject;
OCMInvocationMatcher *invocationMatcher;
BOOL didRecordInvocation;
BOOL shouldReturnMockFromInit;
}

Expand Down
47 changes: 36 additions & 11 deletions Source/OCMock/OCMRecorder.m
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@
#import "OCClassMockObject.h"
#import "OCMInvocationMatcher.h"
#import "OCMRecorder.h"

#import "OCMFunctionsPrivate.h"
#import "OCMMacroState.h"

@implementation OCMRecorder

- (instancetype)init
{
// no super, we're inheriting from NSProxy
didRecordInvocation = NO;
shouldReturnMockFromInit = NO;
return self;
}
Expand Down Expand Up @@ -67,7 +67,7 @@ - (OCMInvocationMatcher *)invocationMatcher

- (BOOL)didRecordInvocation
{
return didRecordInvocation;
return [invocationMatcher recordedInvocation] != nil;
}


Expand All @@ -86,11 +86,22 @@ - (id)ignoringNonObjectArgs
return self;
}

- (BOOL)currentlyRecordingInMacro
{
return [[OCMMacroState globalState] recorder] == self;
}
Comment on lines +89 to +92
Copy link
Owner

Choose a reason for hiding this comment

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

If we move the check from methodSignatureForSelector: to forwardInvocation: then the code in this method is only needed once. I find the name confusing and can't think of a better name. In any case, I think any name wouldn't add much to explain what's happening and given this method is only needed once I'd inline it.


#pragma mark Recording the actual invocation

- (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector
{
if([self didRecordInvocation] && [self currentlyRecordingInMacro])
{
[NSException raise:NSInvalidArgumentException
format:@"Recorder attempting to record `%s` but recorder has already recorded a stub for: `%@`. "
@"Are there multiple invocations on mocks in a single macro?",
sel_getName(aSelector), [self description]];
}
Comment on lines +98 to +104
Copy link
Owner

Choose a reason for hiding this comment

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

Why not move this block into forwardInvocation:? There's another assert there anyway, and I cannot see a benefit for detecting this case here.

if([invocationMatcher recordedAsClassMethod])
return [[(OCClassMockObject *)mockObject mockedClass] methodSignatureForSelector:aSelector];

Expand All @@ -110,18 +121,32 @@ - (NSMethodSignature *)methodSignatureForSelector:(SEL)aSelector

- (void)forwardInvocation:(NSInvocation *)anInvocation
{
NSAssert(!([self didRecordInvocation] && [self currentlyRecordingInMacro]),
@"Should not be recording multiple stubs. This should've been prevented in `-methodSignatureForSelector`.");
[anInvocation setTarget:nil];
didRecordInvocation = YES;
[invocationMatcher setInvocation:anInvocation];

// Code with ARC may retain the receiver of an init method before invoking it. In that case it
// relies on the init method returning an object it can release. So, we must set the correct
// return value here. Normally, the correct return value is the recorder but sometimes it's the
// mock. The decision is easier to make in the mock, which is why the mock sets a flag in the
// recorder and we simply use the flag here.
if([anInvocation methodIsInInitFamily])
// If the method returns an object, we want to return a recorder so that we can trap the case
// where multiple invocations are possibly being recorded in a single OCMStub/OCMExpect macro.
// (this is caught in -methodSignatureForSelector).
if(OCMIsObjectType([[anInvocation methodSignature] methodReturnType]))
{
id returnValue = shouldReturnMockFromInit ? (id)mockObject : (id)self;
id returnValue = self;
// Code with ARC may retain the receiver of an init method before invoking it. In that case it
// relies on the init method returning an object it can release. So, we must set the correct
// return value here. Normally, the correct return value is the recorder but sometimes it's the
// mock. The decision is easier to make in the mock, which is why the mock sets a flag in the
// recorder and we simply use the flag here.
if(shouldReturnMockFromInit && [anInvocation methodIsInInitFamily])
{
returnValue = mockObject;
}
else if ([anInvocation methodIsInCreateFamily])
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure this needs to be in an else branch. Somehow I find the implementation confusing. First we set a "default" return value outside the if statement, then we overwrite it in the if statement, or, alternatively, we manipulate the original value in an else-if. I get why the inCreateFamily check is there, and it's a good additional fix to the change in the PR, but I think I'd restructure this when merging. (No action needed on your part.)

{
// In the case of mocking a create method (new, alloc, copy, mutableCopy) we need to
// add a retain count to keep ARC happy. This will appear as a leak in non-arc code.
[returnValue retain];
}
[anInvocation setReturnValue:&returnValue];
}
}
Expand Down
21 changes: 21 additions & 0 deletions Source/OCMockTests/OCMockObjectMacroTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,27 @@ - (void)testShouldHintAtPossibleReasonWhenVerifyingMethodThatCannotBeMocked
}
}

- (void)testShouldThrowExceptionWhenInvocationsOccurAfterMockRecorded
{
@try
{
id mock = OCMClassMock([NSUUID class]);
OCMExpect([mock UUID]).andReturn(mock);

// Exception should be thrown on this line because we don't want to record [mock UUID],
// we want to record UUIDString.
OCMExpect([[mock UUID] UUIDString]).andReturn(@"Hello");
[NSUUID UUID];
[NSUUID UUID];
OCMVerifyAll(mock);
XCTFail(@"Should never be reached");
}
@catch(NSException *e)
{
XCTAssertTrue([[e reason] containsString:@"but recorder has already recorded a stub for"],
@"Caught Exception: `%@`", [e reason]);
}
}

- (void)testCanExplicitlySelectClassMethodForStubs
{
Expand Down