3
\$\begingroup\$

I have the following method and I would like to understand how it could be reorganized to be more unit-testable.

I think the section about binding to Active Directory could go to its own method, making this method more about either returning bytes or not. 

public byte[] getThumbnail(String username) {
    SimpleBindRequest formInputBindRequest = 
        new SimpleBindRequest(MASTER_USER_DN, MASTER_PASSWORD);
    try {
        LDAPConnection adConnection = getAdConnection();
        adConnection.bind(formInputBindRequest);

        SearchResult searchResults = adConnection.search(properties.getSearchBase(),
                SearchScope.SUB, "(sAMAccountName=" + username + ")", "thumbnailPhoto");
        if (searchResults.getEntryCount() > 0) {
            return searchResults.getSearchEntries()
                                .get(0)
                                .getAttributeValueBytes("thumbnailPhoto");
        }
        return null;
    }
    catch (Exception e)
    {
        return null;
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ The LDAP connection is a dependency, so to make it a unit test you need to mock it. Exposing it is a good choice. Maybe create an interface called: ILightWeightDirectory SearchResult Search(Properties properties); Then either pass it in to the constructor or into the method as a parameter. Then you can create a mock of it easily. Also, this method is a good candidate for an integration test or two when you actually search the directory and bring back the results. \$\endgroup\$
    – Jon Raynor
    Commented Oct 4, 2016 at 13:44
  • \$\begingroup\$ Yeah I was wondering that, are there certain classes that just don't lend themselves to unit testing? I mean this method isn't much without LDAP, right? \$\endgroup\$
    – Paul Duer
    Commented Oct 4, 2016 at 13:48
  • \$\begingroup\$ Yes, so you could forgo unit testing this method and just have a few integration tests instead. If this was the case, I would just mark the tests as boundary crossing so they don't get run as part of the build but instead as a post deployment activity as part of continuous integration. Some methods lend themselves to integration testing so it doesn't make sense to unit test if the code needs major refactoring. I think in this case you could go either way. \$\endgroup\$
    – Jon Raynor
    Commented Oct 4, 2016 at 13:50
  • \$\begingroup\$ I would argue that you shouldn't unit test this code. To do so, you would need to start mocking LDAPConnection and the like. That then leads you down the mocking rabbit hole, where you are testing your mocks, not your production code. I'd favour using some sort of lightweight LDAP service (Microsoft's ADLDS, or Apache Directory are two possibilities) that can then be created and torn down as part of a set of integration tests. That way, you are actually testing your use of LDAP functionality, rather than those mocks. \$\endgroup\$
    – David Arno
    Commented Oct 4, 2016 at 16:08

1 Answer 1

Reset to default
1
\$\begingroup\$

Without changing the code significantly what you need is a seam to insert a different LDAPConnection, and possibly properties. To create the seam for LDAPConnection you've got a couple of different options

  1. Create a class that extends from the current to and overrides getAdConnection()
  2. Constructor inject a LDAPConnectionFactory to the object and use that to replace getAdConnection()

Once you've done one of these you can inject your mock and make some assertions about how the search in being conducted and return a result structure.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.