erikdoe / ocmock

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

Bug: realObject release when partial mocking of the same class. #357

Closed soranoba closed 5 years ago

soranoba commented 6 years ago

Hi, @erikdoe

I specified ~> 3.4.1 in Podfile on my project. Today, I have encountered a problem by auto updating to 3.4.2.

2018-06-26 18 08 00

I found out that realObject of mock1 was released.

https://github.com/erikdoe/ocmock/blob/c4be5d9d9238fcd10449424f651fba2b7aff87c0/Source/OCMock/OCClassMockObject.m#L95-L98

Because, this line called. Would you help me?

Thanks.

soranoba commented 6 years ago

https://github.com/erikdoe/ocmock/compare/v3.4...v3.4.2#diff-7d78571eb78d6c8da84c75e938c87137R98 This change part seems to be affecting, but it crash when revert to restoreMetaClass.

swhitty commented 6 years ago

Yes I have noticed this also.

p1.name = @"Person 1";
p2.name = @"Person 2";
Person *mock1 = OCMPartialMock(p1);
mock1.name == @"Person 1"
Person *mock2 = OCMPartialMock(p2);
mock2.name == @"Person 2"
mock1.name == nil //😱
madsolar8582 commented 6 years ago

I believe that this is a side effect of https://github.com/erikdoe/ocmock/commit/ae40cea3bb4c8bef739022fa96690467f3e6348d since partial mocks also mock their class.

jduquennoy commented 6 years ago

Same analysis for me, it is a side effect of 63aa2bbde90091bfe3c00441673b712e4e541081 part of branche "davidsansome-dispose-first-dymanic-subclass", merged by ae40cea3bb4c8bef739022fa96690467f3e6348d.

erikdoe commented 6 years ago

Thanks for confirming and isolating the change. I will look into this as soon as I can, hopefully early next week.

erikdoe commented 6 years ago

As a progress report: I can reproduce the problem, and I understand what's going on, but it's not clear to me how to resolve it.

First off, though, I noticed that you are looking at the return values from the mock, not the real object. That's a use case I'm not sure I understand. The idea is to use the mock only to control (set up expectations, verify, etc.) and use the real object for everything else.

To verify, I added the following test to OCMock's test suite. (The foo method on the test class is hardcoded to return the String @"Foo".) Note that assertion 1 passes, only 3 fails.

- (void)testSettingUpSecondPartialMockForSameClassDoesNotAffectInstanceMethods
{
    TestClassWithSimpleMethod *object1 = [[TestClassWithSimpleMethod alloc] init];
    TestClassWithSimpleMethod *object2 = [[TestClassWithSimpleMethod alloc] init];

    TestClassWithSimpleMethod *mock1 = OCMPartialMock(object1);
    XCTAssertEqualObjects(@"Foo", [object1 foo]);

    TestClassWithSimpleMethod *mock2 = OCMPartialMock(object2);
    XCTAssertEqualObjects(@"Foo", [object1 foo]);   // 1) passes
    XCTAssertEqualObjects(@"Foo", [object2 foo]);   // 2) passes

    XCTAssertEqualObjects(@"Foo", [mock1 foo]);     // 3) FAILS (value is NULL)
    XCTAssertEqualObjects(@"Foo", [mock2 foo]);     // 4) passes
}

So, if I were to convince you all to only use the real object, not the mock, everything would be allright? Unfortunately not. Have a look at this test:

- (void)testSettingUpSecondPartialMockForSameClassDoesNotAffectStubs
{
    TestClassWithSimpleMethod *object1 = [[TestClassWithSimpleMethod alloc] init];
    TestClassWithSimpleMethod *object2 = [[TestClassWithSimpleMethod alloc] init];

    TestClassWithSimpleMethod *mock1 = OCMPartialMock(object1);
    XCTAssertEqualObjects(@"Foo", [object1 foo]);
    OCMStub([mock1 foo]).andReturn(@"Bar");
    XCTAssertEqualObjects(@"Bar", [object1 foo]);

    TestClassWithSimpleMethod *mock2 = OCMPartialMock(object2);
    XCTAssertEqualObjects(@"Bar", [object1 foo]);   // 5) FAILS (value is Foo)
    XCTAssertEqualObjects(@"Foo", [object2 foo]);   // 6) passes

    XCTAssertEqualObjects(@"Bar", [mock1 foo]);     // 7) passes
    XCTAssertEqualObjects(@"Foo", [mock2 foo]);     // 8) passes
}

It feels like assertion 5 is the real issue here. I'm open to be convinced that assertion 3 is also a problem, even though I almost expect that fixing 5 will also fix 3.

khandpur commented 6 years ago

Hi Eric, any progress on this? As a workaround we're pinning ocmock to 3.4.1 until this is fixed.

tirodkar commented 6 years ago

+1 to this. Any update?

erikdoe commented 6 years ago

Apologies for the delay; this was a tough one. I had been going around in circles trying to dispose the subclass created for mocking class methods at the point when another mock "takes over" mocking class methods. This is problematic because partial mocks create another subclass that depends on the subclass generated for class method mocking. In fact, they don't have to conceptually, but I couldn't find a way to break that dependency given the current runtime implementation.

A couple of days ago I had a new idea, namely to delay the disposing of the subclass created for mocking class methods until the partial mock stops mocking instance methods, ie. when stopMocking is called.

This seems to work and all tests pass, but some confirmations would be nice, given how intricate this change is.

jtoce commented 5 years ago

My developers have hit this if they get in the habit of creating mocks in the -[XCTestCase setUp] method. Here is my example code that repros:

#import <XCTest/XCTest.h>
#import <OCMock/OCMock.h>

@interface OCMockTestClass : NSObject

- (void)methodA;
- (void)methodB;
- (void)doNotCall;

@end

@implementation OCMockTestClass

- (void)methodA {
    [self doNotCall];
}

- (void)methodB {
    [self doNotCall];
}

- (void)doNotCall {
    NSAssert(NO, @"Do not call!");
}

@end

@interface OCMockTestCase : XCTestCase

@property (nonatomic, strong) OCMockTestClass *obj1;
@property (nonatomic, strong) id partialMockObj1;
@property (nonatomic, strong) OCMockTestClass *obj2;
@property (nonatomic, strong) id partialMockObj2;

@end

@implementation OCMockTestCase

- (void)setUp {
    [super setUp];
    self.obj1 = [OCMockTestClass new];
    self.partialMockObj1 = OCMPartialMock(self.obj1);
    self.obj2 = [OCMockTestClass new];
    self.partialMockObj2 = OCMPartialMock(self.obj2);
}

- (void)testExample1 {
    // Will fail with error: -[OCMockTestCase testExample1] : failed: caught "NSInternalInconsistencyException", "Do not call!"
    OCMStub([self.partialMockObj1 doNotCall]);
    [self.obj1 methodA];
}

- (void)testExample2 {
    OCMStub([self.partialMockObj2 doNotCall]);
    [self.obj2 methodB];
}

@end
erikdoe commented 5 years ago

The change has been released in OCMock 3.4.3.