Monday, June 6, 2016

XCTest Assertion Tips

1. Pass actual before expected value to get proper failure messages.
2. Most failures are self-explanatory and should not have a message string.
3. Let the macros do the comparisons so they can report the failure values.
4. Watch for object comparisons done accidentally as pointer comparisons.
5. Use XCTAssertNil with Objective-C objects so that failures will report object values.
6. Report long values at the end of the failure message rather than in the middle.
7. Omit the expectations wait handler block.
8. Use XCTest’s NSNotification and KVO expectations where they can simplify tests of asynchronous behavior.


1. Pass actual before expected value to get proper failure messages.
This is the reverse of typical Java and C++ unit test assertions.
XCTAssertEqual(numberOfDocs, 3);  // Good.
Putting the actual value second yields a backwards failure message, citing the expected value as wrong:
XCTAssertEqual(3, numberOfDocs);  // Avoid; gives this error:
// error: -[MyTests testDocList] : ((3) equal to (numberOfDocs)) 
// failed: ("3") is not equal to ("2")

2. Most failures are self-explanatory and should not have a message string.
Provide failure messages only when they offer helpful additional explanation, or to report related values.
XCTAssert(db.hasCompletedLoading); // Good.
XCTAssert(record.hasLoaded, @”record index %d”, index);  // Good.
These assertion messages are redundant with the standard failure messages:
XCTAssert(db.hasCompletedLoading, @”Database did not load”);  // Avoid.
XCTAssertEqual(count, 3, @”count = %d”, count);  // Avoid. 3. Let the macros do the comparisons so they can report the failure values.
XCTAssertGreaterThan(count, 7);  // Good.
XCTAssertEqualObjects(username, @”jsmith”);  // Good.
XCTAssertEqualObjects(resultsArray, @[ @3, @4, @5 ]);  // Good.
XCTAssertEqualWithAccuracy(rightEdge, 3.2, 0.1);  // Good.
These assertion messages will not report the incorrect values:
XCTAssert(count > 7);  // Avoid.
XCTAssert([username isEqual:@”jsmith”]);  // Avoid.
4. Watch for object comparisons done accidentally as pointer comparisons.
XCTAssertEqualObjects(username, @”mjones”);  // Good.
XCTAssertEqual(username, @”mjones”);  // Avoid (usually).
Explicitly document intentional uses of pointer comparison on objects:
// Use pointer comparison to verify that this is the root of the tree.
XCTAssertEqual(currentObject, rootObject);  // Good.
5. Use XCTAssertNil with Objective-C objects so that failures will report object values.
XCTAssertNil(error);  // Good.
XCTAssert(error == nil);  // Avoid.
6. Report long values at the end of the failure message rather than in the middle.
XCTAssert(file.didLoad,
         @”status %d for file: %@”,
         file.status, file.URL.path);  // Good.
Putting smaller values at the end makes them harder to find in the output log:
XCTAssert(file.didLoad,
         @”file: %@ has status %d”,
         file.URL.path, file.status);  // Avoid.
7. Omit the expectations wait handler block.
The wait call requires a handler block, but the block is not really useful.
The wait invocation blocks the test synchronously, and will assert if an expectation is not fulfilled, so providing a block to check the handler’s error parameter is redundant.
[self waitForExpectationsWithTimeout:1.0 handler:nil];  // Good.
[self waitForExpectationsWithTimeout:1.0 handler:^(NSError *error) {
 XCTAssertNil(error);
}];  // Avoid.
8. Use XCTest’s expectationForNotification: and keyValueObservingExpectationForObject: where they can simplify tests of asynchronous behavior.
[self expectationForNotification:kGTMSessionFetcherStoppedNotification
                         object:myFetcher
                        handler:nil];  // Good.

Tuesday, May 31, 2016

Removing Weak Self Pointers

Blocks brought a variety of sharp edges to Objective-C. The most acute has been the threat of accidental retain cycles due to self references. Explicit calls to self, implicit references such as ivars, and accidental references such as in NSAssert can all easily cause an object to be held indefinitely.

The Dangerous Solution to Cycles

An unfortunately common way to avoid retain cycles in blocks has been reliance on weak self references:
 __typeof(self) __weak weakSelf = self;  
 [object prepareVacationWithBlock:^{  
  __typeof(weakSelf) strongSelf = weakSelf;  
  [strongSelf vaccinateDog];  
  [kennel reserveDogSpace];  
  [strongSelf hireCatSitter];  
 }];  
A fundamental problem with this approach is that weak self references create code that in the user’s hands may not execute at all like expected or tested.
For example, during development, it’s typical for the weakSelf in the block above to never be nil. But in a release build, objects may be more aggressively released, so the weakly-referenced object may be released before the block executes. When the example block then runs on the user’s device, strongSelf is occasionally nil, and kennel is getting reserved even though the dog didn’t get vaccinated.
Blocks retain references to ensure that each block’s dependencies are still around when the block code executes. Reliance on weak self references subverts these semantics, making the code that actually runs in the block unpredictable. Use of weak selfs turns a typically minor problem, a small retain cycle that ought to be caught in Instruments during app development, into a much more serious problem of untested code paths that crop up just on end-user machines. A small, predictable leak is replaced by an elusive crash or hang.
Use of weak self references has been dressed up with “weakify” macros. Those formalize the approach of ignoring the problem rather than fixing it.

Safety by Design

So what’s a better approach? Design APIs that cannot cause retain cycles. Class properties and method parameters should never take a block pointer unless the API provider can guarantee there won’t be a retain cycle created. Safe use patterns should be inherent in the interface; they shouldn’t be left up to the understanding or paranoia of every client trying to use the API.
When blocks passed to a class can’t be guaranteed to not cause retain cycles, then the class should have zeroing weak object pointers for its callbacks, not block pointers. It is the class’s responsibility to vend a safe API, rather than a challenge for every caller to try to get right. Blocks are often chosen for callbacks for the short-term design advantage, ignoring the long-term cost they impose on client code.
Still, some designs unfortunately do require blocks. The simplest way a method can ensure that blocks passed as parameters won’t lead to retain cycles in the client code is to only hold blocks in local variables. Since blocks pointed to from the stack are certain to be released as the stack unwinds, they’re guaranteed to release their references as well.
Often, a class will have to hold on to a block for use later, either as a completion handler or for repeated use as a monitoring callback. In either case, the class should be written to ensure that all blocks are released at a specific time in the near future. The example class below does that.
Waiting for the class to deallocate to release its blocks just leaves class’s users in a battle against retain cycles.


 @interface CatVideoDownloader : NSObject <CatVideoFetcherDelegate>  
   
 - (void)fetchCatVideoWithCompletionHandler:  
   (void (^)(NSData *kittenData, NSError *error))handler;  
   
 // The optional progress block may be called repeatedly.  
 @property(nonatomic, copy)   
   void (^progress)(uint64_t numberOfBytesReceived);  
 @end  
   
 @implementation CatVideoDownloader {  
  // We'll hold this completion handler only until the fetcher  
  // finishes or fails. When either happens, we call back to  
  // the client appropriately, and release all block ivars.  
  void (^_completionHandler)(NSData *kittenData, NSError *error);  
 }  
   
 - (void)fetchCatVideoWithCompletionHandler:  
   (void (^)(NSData *kittenData, NSError *error))handler {  
  // Start downloading with this object as the fetcher's delegate.  
  _completionHandler = [handler copy];  
  [CatVideoFetcher startFetchingCatVideoWithDelegate:self];  
 }  
   
 - (void)fetcher:(CatVideoFetcher *)fetcher  
   didReceiveBytes:(uint64_t)numberOfBytesReceived {  
  // Update the client on progress.  
  if (_progress) {  
   _progress(numberOfBytesReceived);  
  }  
 }  
   
 - (void)fetcher:(CatVideoFetcher *)fetcher  
   finishedWithData:(NSData *)kittenData  
         error:(NSError *)error {  
  // Downloading finished or failed; call back and release blocks.  
  [self invokeCallbackAndCleanUpWithData:kittenData  
                   error:error];  
 }  
   
 - (IBAction)userPressedCancel:(id)sender {  
  // User cancelled; call back and release blocks.  
  NSError *error =   
   [NSError errorWithDomain:@"com.example.CatVideoDownloader"  
             code:NSURLErrorCancelled  
           userInfo:nil];  
  [self invokeCallbackAndCleanUpWithData:nil error:error];  
 }  
   
 - (void)invokeCallbackAndCleanUpWithData:(NSData *)kittenData  
                   error:(NSError *)error {  
  // Ensure the handler ivar is nil before calling the handler  
  // so reentrancy won't lead to the callback being called twice.  
  void (^callback)(NSData *, NSError *) = _completionHandler;  
  _completionHandler = nil;  
  if (callback) {  
   callback(kittenData, error);  
  }  
   
  // We’re done, so release all other blocks too.  
  _progress = nil;  
 }  
   
 @end  

Monday, May 30, 2016

Completion Handlers

Have a single completion callback.

A completion handler interface should pass both the results and any errors:

- (void)chewTheBolusWithCompletionHandler:
   (void (^)(id result, NSError *error))callback;

It’s tempting to have separate success and failure callbacks. After all, apps will be handling each separately, right? It’s tempting, but it's dangerous. Success and failure cases both require cleanup by the app. Separating the handlers means cleanup will likely be omitted by accident for one of the callbacks.

Completion code should be called exactly once.

It's far easier for clients to use a simple, predictable interface. When a call takes a completion handler, be it a block or a selector, the handler should be called exactly once.  Calling zero times, or more than once, leaves clients unable to easily clean up their state.

Propagate the underlying error to the callback.

The underlying error should be passed back explicitly, especially from any code below the topmost UI level. If necessary, synthesize an NSError. Only the UI layer will know how to present the failure; for example, “server timeout” and “cannot reach host” may or may not need to be presented to users. The UI layer can make an appropriate decision only with an error to examine.

Some NSFileManager APIs originally just returned BOOLs to indicate success or failure, but those needed to be deprecated and replaced with versions that pass back NSErrors. Application code really does need to know the error that occurred.

Indicate failure only through the callback, not through a return value.

Methods that take a callback pointer should not also return a value. Rather, if they can fail, they should always call the callback, so the client only needs to handle failures in one place, not two.

Call back in a consistent manner for success and failure.

Most code succeeds often and fails rarely (or vice versa.) Since callbacks are typically tested more in one case than the other, each method should strive to always call back on a consistent thread or queue.

For example, if a method would succeed asynchronously and then call back on the main thread, it should be written so it also fails and calls back with an error asynchronously on the main thread. Consistency avoids bugs due to some unexpected, untested app state during the callback. A dispatch_async is fast, so the overhead is negligible.

 - (void)makeBrowniesWithCompletion:(void (^)(id brownies,  
                   NSError *error))callback {  
  if (!_hasIngredients) {  
   // We can’t make brownies. Call the callback with an error on the  
   // same queue as we would use if baking was successful.  
   dispatch_async(dispatch_get_main_queue(), ^{  
    NSError *ingredientError =   
     [NSError errorWithDomain:kMyAppDomain  
               code:kMyAppIngredientErrorCode  
             userInfo:nil];  
    callback(nil, ingredientError);  
   });  
   return;  
  }  
   
  [self preheatOvenAsyncWithCompletion:^{  
   // Preheating calls back on the main queue.  
   NSError *bakeError;  
   id brownies = [self bakeIngredientsWithError:&bakeError];  
   callback(brownies, bakeError);  
  }];  
 }  
 - (void)makeBrowniesWithCompletion:(void (^)(id brownies,  
                   NSError *error))callback {  
  if (!_hasIngredients) {  
   // We can’t make brownies. Call the callback with an error on the  
   // same queue as we would use if baking was successful.  
   dispatch_async(dispatch_get_main_queue(), ^{  
    NSError *ingredientError =   
     [NSError errorWithDomain:kMyAppDomain  
               code:kMyAppIngredientErrorCode  
             userInfo:nil];  
    callback(nil, ingredientError);  
   });  
   return;  
  }  
   
  [self preheatOvenAsyncWithCompletion:^{  
   // Preheating calls back on the main queue.  
   NSError *bakeError;  
   id brownies = [self bakeIngredientsWithError:&bakeError];  
   callback(brownies, bakeError);  
  }];  
 }  

Prevent accidental repeat callbacks.

It's safest if a callback pointer does not need to be stored in an ivar or a static variable. Block retain cycles won't happen when block pointers are in local variables. Try to structure your code to hold callback pointers just in local variables, and be especially wary of ever using a global or static variable for a block.

But if a callback pointer is stored, then remove the stored value before invoking the callback.

 - (void)invokeCallbackWithResult:(id)result  
               error:(NSError *)error {  
  dispatch_async(dispatch_get_main_queue(), ^{  
   void (^callback)(id result, NSError *error) = self.callback;  
   self.callback = nil;  
   if (callback) {  
    callback(result, error);  
   }  
  });  
 }  

Clearing the ivar or static before invoking the callback guarantees that nothing the callback does can lead to a repeat of the completion handler call.

Note too that in this sample, the callback is grabbed from the ivar inside the same block in which the callback block is called. This avoids creating an opportunity during the dispatch_async delay for the client code to cancel the operation and clear the callback property.

A window of opportunity during the dispatch_async might lead to the callback being invoked despite the operation having just been canceled if the callback pointer was retained in a local variable before the dispatch_async block. This may be appropriate if cancel shouldn’t stop the callback, though most often, callbacks aren’t desirable after an operation has been canceled.

Document, document, document.

Comments for the class, or for each method in the class, should describe the thread or queue expectations for callers of the methods, and the thread or queue used when invoking the method’s callback. Your future self will thank you for writing it down.