Unit testing similar wrappers - or single unit test is enough?

Question

I'm working on a service which has a lots of dependencies. The way how I call them is I'm wrapping each service client around an Adapter. Like this (I'm using Java):

public abstract class AdapterBase<Request, Response> {
    protected abstract String adapterMetadata();
    protected abstract Response makeActualCall(Request request);

    // additional things are happening here, for each dependency call it's the same
    public Optional<Response> call(Request request) {
        try {
            return Optional.ofNullable(makeCall(request));
        } catch(Exception ex) {
            log.error("bad thing happened", ex);
            return Optional.empty();
        }
    }
}

Now, for each API I'm calling, I'm creating a new adapter, and for each adapter, I'm creating a new unit test.

A sample implementation of this AdapterBase:

public class FooAdapter<FooRequest, FooResponse> extends AdapterBase<FooRequest, FooResponse> {
    private FooClient fooClient;            
    public FooAdapter(FooClient fooClient) {
        this.fooClient = fooClient;
    }

    @Override
    protected String adapterMetadata() {
        "Calling Foo";
    }

    @Override
    protected FooResponse makeActualCall(FooRequest fooRequest) {
        return fooClient.call(fooRequest);
    }
}

And I have the similar inherited classes for each API. The basic unit test for this concrete FooAdapter would be the following:

@RunWith(MockitoJUnitRunner.class)
public class FooAdapterTest {
    @Mock
    private FooClient fooClient;

    @Mock
    private FooResponse fooResponse;

    private FooAdapter fooAdapter;

    @Before
    public void setup() {
        fooAdapter = new FooAdapter(fooClient);
    }

    @Test
    public void whenExceptionIsThrownThenEmptyObservableReturns() {
        when(fooClient.call(any()).thenThrow(FooException.class);

        Optional<FooResponse> actual = fooAdapter.call(new FooRequest());

        assertFalse(actual.isPresent());
    }

    @Test
    public void whenCallSucceededThenResultReturns() {
        when(fooClient.call(any()).thenReturn(fooResponse);

        Optional<FooResponse> actual = fooAdapter.call(new FooRequest());

        assertTrue(actual.isPresent());
        assertEquals(fooResponse, actual.get());
    }
}

And for all the concrete adapters, I have the same unit tests. At this point, I'm wondering if these unit tests are holding any value to us. It's all the same, the only difference is that different "clients" can throw different exceptions, but because of the implementation of AdapterBase, it doesn't really matter what exception it was.

Having these classes are obviously increasing test-coverage metric, but I'm not 100% convinced on what value we are getting out of it. I'm thinking of changing this to one artificial implementation of the AdapterBase and I only have one single unit test against it, since all the adapters are following the same pattern.

I was reading that too many very low-level unit tests might be an obstacle against agile changes since it requires lots of code changes in non-production-related codebase. However, each adapter is a single unit, therefore it has to be tested.

I'm curious about what would be the right way to follow in this case.

Answer 1

I would hesitate before not testing even "trivial" code. Sure this code may be covered by non-unit automated tests, in which case you could argue that that coverage is sufficient to show the correctness of your program. But in the grand scheme of things unit tests are simpler to reason about, execute much faster, and are more precise in locating the exact issue (if written well).

More importantly that test is active documentation. Even if i'm unaware of its specific existence when i do make a change to this code later, that unit test informs me if i have broken an expectation. Its pretty obvious that the logging is not a requirement, if i deleted it tomorrow the system should still work, i.e. I could deploy it without anyone complaining. The only hard requirement is that an empty optional is returned on error, and that otherwise there must be a response object.

But looking at your code, yes there is a lot of boiler plate here. So perhaps refactoring your code to exclude the boiler plate is a better outcome.

1. Can you just use fooClient.call(request) instead of needing an adaptor? - If yes then the adaptor and unit test disappear in smoke. A celebration happens because code was deleted.
2. Is the adapter providing any useful abstraction? - Probably not, its not hiding any type information, nor is it contributing to your domain knowledge. It does indeed look like a filler class. (not necessarily filler logic though).
3. Is the adapter providing any additional behaviour? - Maybe its trapping errors, providing some rudimentary meta-data, and generally acting as a boundary type. If the underlying type does all of that this, then logic is duplicated, See if it can be deleted.
4. Is the the adaptor mostly boiler plate? - Yes definetly, the knowledge in it could be essential reduced down to Adaptor("meta-data", request -> fooClient.call(request)) and that line could be stored within an sort of IoC, Factory, hard-coded procedure.

Java has lambda syntax. https://docs.oracle.com/javase/tutorial/java/javaOO/lambdaexpressions.html

One possible re-write:

public class Adapter<Request, Response> {
    protected String metadata;
    protected Function<Request, Response> adapted;

    public Adapter(String metadata, Function<Request, Response> adapted)
    {
        this.metadata = metadata;
        this.adapted = adapted;
    }

    public Optional call(Request request) {
        try {
            return Optional.ofNullable(adapted(request));
        } catch(Exception ex) {
            log.error("bad thing happened", ex);
            return Optional.empty();
        }
    }
}

Now you have one class to test for actual logic.

And then somewhere else:

List a = new List();
a.Add(new Adaptor("meta-data", request -> fooClient.call(request)));
a.Add(new Adaptor("meta-data", request -> fooClient2.call(request)));
a.Add(new Adaptor("meta-data", request -> request.prop ? null : fooClient2.call(request)));

You'll still need to exercise these lambdas, but it might just so be easier in the factory context anyway.

Another rewrite is to phrase the adapter itself as a lambda in the form Function<Request, Response>. This would then boil down to one lambda per adapted service

request -> {
    try {
        return Optional.ofNullable(adapted(request));
    } catch(Exception ex) {
        log.error("bad thing happened", ex);
        return Optional.empty();
    }
}

This won't reduce the amount of testing, it still has the same surface area as the adaptor class, but there is definitely less boilerplate, and opens up ways to simplify configuration. (which could potentially be a bigger win).

Answer 2

I get paid for code that works, not for tests, so my philosophy is to test as little as possible to reach a given level of confidence -- Kent Beck.

Tony Hoare talked about code that "obviously has no deficiencies." My usual spelling is "too stupid to fail".

How often are you editing these implementations? How often are you actually finding bugs in them? Are the tests providing extra value as documentation? Are they still contributing to your design processes?

Writing a test costs money. Maintaining a test costs money. Running a test costs money. If the tests aren't giving you anything back, that money is wasted.

In writing [Why Most Unit Testing is Waste], Jim Coplien observed that low risk tests have low (potentially negative) payoff. He describes a number of categories of test where the value prop is suspect -- I suspect you will find that the tests you are questioning here fall into one or more of those categories.

Horses for courses.