Leak in FBLPromiseDelayTests.testPromiseDelaySuccess?

[grigorye]

[Disclaimer: I'm exercising a solution for automatic leak detection in unit tests. Promises was the first framework that I was able to find that has test_spec and suitable tests.]

It looks like potentially there's a FBLPromise leak in FBLPromiseDelayTests.testPromiseDelaySuccess. It is reproducible for both iOS and macOS unit tests.

Test Suite 'Selected tests' started at 2018-12-13 04:17:56.372
Test Suite 'PromisesObjC-iOS-Unit-Tests.xctest' started at 2018-12-13 04:17:56.376
Test Suite 'LeaksTestCase' started at 2018-12-13 04:17:56.377
Test Case '-[GELeaks.LeaksTestCase testLeaks]' started.
FBLPromiseDelayTests.testPromiseDelaySuccess: FBLPromise is likely leaked 1 times.
FBLPromiseDelayTests.testPromiseDelayFail: FBLPromise is potentially leaked.
Test Case '-[GELeaks.LeaksTestCase testLeaks]' passed (210.134 seconds).
Test Suite 'LeaksTestCase' passed at 2018-12-13 04:21:26.515.
     Executed 1 test, with 0 failures (0 unexpected) in 210.134 (210.137) seconds
Test Suite 'PromisesObjC-iOS-Unit-Tests.xctest' passed at 2018-12-13 04:21:26.518.
     Executed 1 test, with 0 failures (0 unexpected) in 210.134 (210.142) seconds
Test Suite 'Selected tests' passed at 2018-12-13 04:21:26.519.
     Executed 1 test, with 0 failures (0 unexpected) in 210.134 (210.147) seconds

If you want to reproduce exactly like I reproduced it:

1. Check out my fork - there's a change just in podspec + one (empty) .swift file added to Tests (to enable Swift support).
2. Make sure you have gems for cocoapods (1.6.0 beta 2) and cocoapods-generate installed
3. Do the following to generate gen/PromisesObjC/PromisesObjC.xcworkspace:

   pod gen --sources=https://github.com/grigorye/podspecs.git,https://github.com/CocoaPods/Specs.git

4. Open gen/PromisesObjC/PromisesObjC.xcworkspace
5. In Test action PromisesObjC-iOS-Unit-Tests scheme disable all tests but "LeaksTestCase".
6. Run the tests.

[grigorye]

OK, it looks like it's not a leak rather side effect observed after completion of -testPromiseDelaySuccess: the promise leaked remains retained for about 1 second due to FBLDelay(1, ...). Will do PR that fixes that.

[shoumikhin]

Hi Grigory,

Thank you for taking a closer look at the tests and checking the lib for leaks!

Please keep in mind that test is designed to retain the promise for an extended period of time (1 sec) and tries to reject it afterwards, which normally has no effect because the promise should have been already resolved after the delay of 0.1 sec specified in the delay operation. So the promise stays retained by GCD in FBLDelay after the test succeeds. Not sure why the leaks checker flags that as an issue, but that’s pretty much by design.

To satisfy the leak checker we need to make sure the FBLDelay block has a chance to complete before the test has ended (by using XCTestExpectation, for example). But I’m not sure it’s worth doing really, since it doesn’t add any value to the test and the test doesn’t leak either. WDYT?

Thanks.

[grigorye]

Yep, I believe I understand the design of the test. The leaker flags it as a leak because it's naive in assumption that there should be no objects retained after the test execution (and it flags any such an object as a "likely leaked" one).

As for "whether it's worth fixing it" - I'm not sure about your attitude to this, but I would fix it - it's clearly a side effect observed after the test execution. As a side effect it might affect the code (in this case, other tests) executed afterwards. Actually, if I get it correctly, if there's a test afterwards, -reject: will be executed in context of that next test (or a test after the next test - imagine it). If -reject: has a problem (e.g. crash or bug), it won't be revealed by -testPromiseDelaySuccess at all, it will affect some other test and etc. So, it looks like it's not only "delayed retain" but potentially "test reliability" problem.

Let me illustrate it.

I added dummy test that would be executed after -testPromiseDelaySuccess (the tests seem to be executed in lexicographical order, so all other cases in FBLPromiseDelayTests are executed before -testPromiseDelaySuccess):

- (void)testPromiseDelaySuccessOtherWay {
  [[NSRunLoop currentRunLoop] runUntilDate:[NSDate dateWithTimeIntervalSinceNow:5]];
}

Added %H %B @self@ non-stopping debugger log action on -initXXX, -dealloc and -reject: in FBLPromise.

Selected just -testPromiseDelaySuccess and -testPromiseDelaySuccessOtherWay in schema tests.

Run test action:

Test Suite 'FBLPromiseDelayTests' started at 2018-12-14 08:08:54.235
Test Case '-[FBLPromiseDelayTests testPromiseDelaySuccess]' started.
1 -initWithResolution: 0x7f955e4c26a0
1 -initPending 0x7f955e4c2800
2 -initPending 0x7f955e4c29d0
3 -initPending 0x7f9560815260
1 -dealloc 0x7f955e4c26a0
2 -dealloc 0x7f955e4c29d0
3 -dealloc 0x7f9560815260
Test Case '-[FBLPromiseDelayTests testPromiseDelaySuccess]' passed (1.310 seconds).
Test Case '-[FBLPromiseDelayTests testPromiseDelaySuccessOtherWay]' started.
1 -reject: 0x7f955e4c2800
4 -dealloc 0x7f955e4c2800
Test Case '-[FBLPromiseDelayTests testPromiseDelaySuccessOtherWay]' passed (5.003 seconds).
Test Suite 'FBLPromiseDelayTests' passed at 2018-12-14 08:09:00.551.

As you can see, we get -reject: for promise allocated in -testPromiseDelaySuccess executed as part of (completely unrelated/next) -testPromiseDelaySuccessOtherWay (alongside -dealloc). That's not a good thing in my opinion...

[shoumikhin]

Thank you for the detailed explanation! That makes sense to me. Wanna make a PR? Thanks.