Unit testing – only pass it what you need

Leave a comment

I was reminded again today how much better your code can be if only pass in those variables you need.

Here’s where I started.

 [self.fbMessenger shareViaAppExtensionEntityData:self.viewModel.data.entityData utmTagsEnabled:utmTagsEnabled];
- (void)shareViaAppExtensionEntityData:(SPTShareEntityData *)entityData utmTagsEnabled:(BOOL)utmTagsEnabled
{
    // create fb messenger share URL
    NSString *facebookAppExtensionURL = entityData.webURI.absoluteString;

    if (utmTagsEnabled) {
        facebookAppExtensionURL = [self facebookAppExtensionURLForEntityDataURL:entityData.webURI.absoluteString];
    }

    // share
    FBSDKShareLinkContent *content = [[FBSDKShareLinkContent alloc] init];
    content.contentURL = [NSURL URLWithString:facebookAppExtensionURL];
    [FBSDKMessageDialog showWithContent:content delegate:nil];
}

But what’s wrong with this method. It’s passing in too much! I don’t need the full entity. Just the string.

My trick to catching this like this is to consciously refactor. Every 10 or 15 mins, or every time I think I am done a feature, go back and review your code.

These are the questions you continuously want to be asking yourself:

  1. Is this the simplest code possible.
  2. Am I only passing into methods that which I need.
  3. Does this read well? Am I going to understand what I have written two weeks from now.
  4. Is each method only doing one thing.

Here’s where I ended up after some more refactoring.

[self.fbMessenger shareEntityURL:entityURL utmTagsEnabled:utmTagsEnabled];
- (void)shareEntityURL:(NSString *)entityURL utmTagsEnabled:(BOOL)utmTagsEnabled
{
    // create URL to share
    NSString *urlToShare = [self urlToShareForEntityURL:entityURL utmTagsEnabled:utmTagsEnabled];

    // share
    FBSDKShareLinkContent *content = [[FBSDKShareLinkContent alloc] init];
    content.contentURL = [NSURL URLWithString:urlToShare];
    [FBSDKMessageDialog showWithContent:content delegate:nil];
}

- (NSString *)urlToShareForEntityURL:(NSString *)entityURL utmTagsEnabled:(BOOL)utmTagsEnabled
{
    NSString *urlToShare = entityURL;

    if (utmTagsEnabled) {
        urlToShare = [self addUTMTagsToURL:entityURL];
    }

    return urlToShare;
}

By pulling out the if statement into a method dedicated to building the URL, I get away from the old method from doing x2 things instead of one.

Now it’s much easier to test. I can write a test that adds UTM tags, and another that doesn’t.

Example of method doing too much – unit testing

Leave a comment

Was working on a refactor of this method when I realized it was doing too much.

- (void)showAppropriateView
{
    if ([self isOffline]) {
        [self.viewManager showOfflineView];
    } else if (!self.nullstateScreenHasBeenDisplayed || [self searchBarHasNoText]) {
        [self.viewManager showNullstateView];
    } else if (self.errorHasOccurred) {
        [self.viewManager showErrorView];
    } else if (self.tracks.count > 0) {
        [self.collectionView reloadData];
        [self.viewManager showContentView];
    } else {
        [self.viewManager showNoResultsView];
        [self updateNoResultsSearchText];
    }
}

On the surface it doesn’t look that bad. Just figuring out what view to display, and then displaying it.

But there are really x2 things going on with this method. One is displaying the view. But the other is doing the other things as a result of that view being displayed.

Before breaking I had to pull out the non display view stuff. Then I was free to have another method focus on the view logic itself.

Final result looked like this.

- (void)showAppropriateView
{
    if (self.tracks.count > 0) {
        [self.collectionView reloadData];
    } else if (self.tracks.count == 0) {
        [self updateNoResultsSearchText];
    }

    [self.viewManager showAppropriateViewWhenOffline:[self isOffline]
                     nullstateScreenHasBeenDisplayed:!self.nullstateScreenHasBeenDisplayed
                                  searchBarHasNoText: [self searchBarHasNoText]
                                    errorHasOccurred:self.errorHasOccurred
                                          trackCount:self.tracks.count];
}

Unit testing without mocks 1

Leave a comment

Here is an example of how to remove a dependency and only pass in what you need. Avoids mocking Notification. Instead just extract value.

+ (CGFloat)constraintForKeyboardWithHeight:(CGFloat)keyboardHeight
                                viewHeight:(CGFloat)viewHeight
                               imageHeight:(CGFloat)imageHeight
                               labelHeight:(CGFloat)labelHeight;
{
    if( IS_IPHONE_5 || IS_IPAD )
    {
        navigationBarHeightPortrait = 64;
    }

    CGFloat errorMesageHeight = [self errorMessageHeightForImageHeight:imageHeight labelHeight:labelHeight];
    CGFloat topConstraintHeightPortrait = (viewHeight - keyboardHeight - errorMesageHeight - navigationBarHeightPortrait - bottomMessaageBarHeight)/2;

    // We are currently ignoring device orientation and only returning portrait
    return topConstraintHeightPortrait;
}

#pragma mark Private

+ (CGFloat)keyboardHeightForNotification:(NSNotification *)notification
{
    NSDictionary *userInfo = [notification userInfo];
    NSValue *keyboardFrameBegin = [userInfo valueForKey:UIKeyboardFrameBeginUserInfoKey];
    CGRect keyboardFrameBeginRect = [keyboardFrameBegin CGRectValue];
    CGFloat keyboardHeight = keyboardFrameBeginRect.size.height;
    return keyboardHeight;
}

Unit testing tip – additional override

1 Comment

If you have have a method you want to test, but it does some stuff you like for convenience, and you don’t really want to mess for the sake of testing, create with some overrides and do your testing there.

+ (void)foo:(NSURLSessionConfiguration *)sessionConfiguration
{
    TokenManager *tokenManager = [[TokenManager alloc] init];
    AccessToken *accessToken = tokenManager.accessToken;

    [self addAuthorizationHeaderToSessionConfiguruation:sessionConfiguration token:accessToken];
}

+ (void)foo:(NSURLSessionConfiguration *)sessionConfiguration token:(AccessToken *)accessToken
{
    NSString *accessTokenHeaderValue = [NSString stringWithFormat:@"%@ %@", accessToken.tokenType, accessToken.token];
    sessionConfiguration.HTTPAdditionalHeaders = @{@"Authorization" : accessTokenHeaderValue};
}

The first method is the convenient public one.
The second is the one you can test and inject other things into.

Testing protected methods

Leave a comment

In my early unit testing days, if there was a method I wanted to test I would simply expose it and access it in my unit test like this.

public class SingleLinkedListTest {

  @Test
  public void pushFrontOneElement() throws Exception {

      SingleLinkedList<String> list = new SingleLinkedList<String>();
      list.pushFront("a");

      Assert.assertEquals(1, list.size());
      Assert.assertEquals(list.topFront(), "a");
      Assert.assertEquals(list.head(), list.tail());

  }
}

list.head() and list.tail() are internal details of how this LinkedList was built. These shouldn’t be exposed though the public API.

Yet I really want to write some kind of test that forces me to write code like this:

public class SingleLinkedList<T> {
  public void pushFront(T data) {
      Node<T> oldHeader = head;
      head = new Node<T>(data, oldHeader);
      if (size == 0) {
          tail = head;
      }
      size++;
  }
}

What I’ve come to realize over the years is that while writing highly coupled internal unit tests like this isn’t wise, it’s OK if you use it as a temporary step to get you going.

You see early in an APIs life you have a chicken and an egg problem. The public methods you’d ideally like to use to test a method don’t yet exist. But you still some kind of way to test what you are doing. Exposing internals temporarily I think is OK to get you going.

Then you can always come back, and replace the internal method calls with public ones like this.

@Test
public void pushFontTwoElements() throws Exception {

  list.pushFront("a");
  list.pushFront("b");

  Assert.assertEquals(2, list.size());
  Assert.assertEquals(list.topFront(), "b");
  Assert.assertEquals("b", list.popFront());
  Assert.assertEquals("a", list.popFront());
}

So don’t be afraid to test internals if it gets you going. Just don’t forget to come back and replace them once you’ve got your other public APIs in place. This get’s you past the chicken and egg problem. While also eventually leaving you with a nice publicly tested API.

%d bloggers like this: