What would be an ideal unit test in this simple case?

Question

I have a simple service that fetches an article from the repository. If it finds it then it returns it otherwise throws an exception. In short the function is totally dependent on the database interactions.

public class ArticleService {

    private ArticleRepository articleRepo;

    public Article getArticle(Long articleId) {
       Optional<Article> responseArticle = articleRepo.findById(articleId);
       Article article = responseArticle.orElseThrow(() -> new NoSuchArticleException(ExceptionMessages.ARTICLE_NOT_FOUND.getMessage()));
       return article;
    }

}

Now when I have to write a unit test for ArticleService I will stub the result from the database and assert accordingly.

@Test(expected=NoSuchArticleException.class)
public void getArticleTestWhenArticleNotPresent() {
    when(articleRepo.findById(anyLong())).thenReturn(Optional.ofNullable(null));
    articleService.getArticle(1L);
}

@Test
public void getArticleTestWhenArticlePresent() {
    Article expectedArticle = new Article(1L, "some Article");
    when(articleRepo.findById(anyLong())).thenReturn(Optional.of(expectedArticle));
    Optional<Article> actualArticle = articleService.getArticle(1L);
    Assert.equals(actualArticle, expectedArticle);
}

Here even the unit tests seem to be dependent on the stubs. A part of my brain says that this is an integration test rather than unit test. Moreover if I change the method findById() to take int rather than long, then these tests won't compile. In ideal unit test scenarios changes in other unit should not affect the unit test. I know the definition of unit tests is loosely defined but what can be a unit test for this scenario? The more I think the more I get confused. I even feel that the only thing being tested here is the return construct of the language.

Answer 1

Here even the unit tests seem to be dependent on the stubs. A part of my brain says that this is an integration test rather than unit test.

ArticleService is dependent on an instance of ArticleRepository. Whether a test involving ArticleService is a unit or integration test comes down to, "does it have side effects". Use a real repository and it does. SO that's an integration test. Use stubs like you show and it doesn't. So that's a unit test.

Moreover if I change the method findById() to take int rather than long, then these tests won't compile

And? Nor will ArticleService. Your tests are coupled to ArticleRepository through the need to mock it. So if you make a breaking change to the latter's public API, you have to expect the former to fail to compile.

In ideal unit test scenarios changes in other unit should not affect the unit test.

Which is the case here. They are behaving as unit tests should.

I even feel that the only thing being tested here is the return construct of the language.

You are testing that if the ID is found in the repository, the value is returned, otherwise an exception is thrown. Assuming that is what your getArticle function is supposed to do, then your tests are good as they cover both scenarios.

Answer 2

If you are unit testing ArticleService you should inject a mock ArticleRepository. No need for a database.

But, as you say, you have almost zero logic in ArticleService. So the test doesnt test much.

If you are testing ArticleRepository on the other hand, mocking the database probably isn't something you want to do. Sure you will have some deserialisation logic in there, but a large part of the functionality will be in the SQL.

You can have your test create a database specifically for the test run, or use a common test database but now most people would call it an integration test.

Answer 3

A trick I find useful is to look for functions.

Presumably, if we were to unroll your test setup functions, we would see something like:

articleService = new ArticleService(articleRepo);
// ...
Article article = articleService.get(1L)

That really isn't all that different from this idea

Article article = someNewFunction(1L, articleRepo);

In your current design, we could implement that idea pretty trivially:

Article someNewFunction(Long articleId, ArticleReport articleRepo) {
    ArticleService service = new ArticleService(articleRepo);
    return service.get(articleId)
}

But what if we inverted that idea - do the real work in the function, and call into it from the service?

class ArticleService {
    // ...
    Article get(Long articleId) {
        return someNewFunction(articleId, this.articleRepo);
    }
}

The article service piece is at this point just a "closure", an adapter that allows us to clearly distinguish data with a short lifetime (the articleId) from the capabilities that persist.

There's a second separation that you can do in languages, like java, that allow you to pass "functions" as arguments....

Article someNewFunction(Long articleId, ArticleReport articleRepo) {
    return someOtherNewFunction(articleId, articleRepo::findById);
}

Article someOtherNewFunction(Long articleId, Function<Long, Optional<Article>> find) {
   Optional<Article> responseArticle = find.apply(articleId);
   Article article = responseArticle.orElseThrow(() -> new NoSuchArticleException(ExceptionMessages.ARTICLE_NOT_FOUND.getMessage()));
   return article;
}

What we've done here is divide the design into two parts. The parts that know about the repository are hard to test, but also have the property that they are "so simple there are obviously no deficiencies". someOtherNewFunction does real work -- but also has the property that it is easy to test.

If you hate the spelling of find.apply enough (I do) you can consider instead introducing a better name for the semantic:

interface Find {
    Optional<Article> byId(Long articleId);
}

Article someOtherNewFunction(Long articleId, Find find) {
   Optional<Article> responseArticle = find.byId(articleId);
   Article article = responseArticle.orElseThrow(() -> new NoSuchArticleException(ExceptionMessages.ARTICLE_NOT_FOUND.getMessage()));
   return article;
}

Part of the point of tdd is that "making the code easier to test" is a first class design concern. So this kind of a refactoring is a normal thing to do in that setting.

Moreover if I change the method findById() to take int rather than long, then these tests won't compile. In ideal unit test scenarios changes in other unit should not affect the unit test.

That's not quite right. Changes to hidden implementation details shouldn't affect your tests, but backwards incompatible changes to published interfaces are a different story. That "ought" to be catastrophic.

However, backwards incompatible changes do happen. So let's consider this last case - what happens if we MUST change the interface for the ArticleRepository?

Article someOtherNewFunction(Long articleId, Function<Long, Optional<Article>> find)

This code is perfectly fine; all of the tests that we wrote against it still pass. By good fortune, some happy design heuristics, and luck, we happened to write a bunch of tests that were isolated from the unstable dependency.

return someOtherNewFunction(articleId, articleRepo::findById);

This bit of code fails to compile. However, it's pretty easy to change the ArticleService so that the code will compile...

Article someOtherNewFunction(Long articleId, Function<Integer, Optional<Article>> find) {
   Optional<Article> responseArticle = find.apply(articleId);
   Article article = responseArticle.orElseThrow(() -> new NoSuchArticleException(ExceptionMessages.ARTICLE_NOT_FOUND.getMessage()));
   return article;
}

This is, again, a function that is easy to test. Furthermore, we can notice "oh, there is duplication with our earlier code", and do some refactoring.

In a situation where you have a language without type checking and a programming team without discipline about backwards incompatible changes, you've got a real problem. I'd recommend pulling the Andon cord and getting the entire team together to hash things out.

It's also worth noting that, even in a language with a type checker, there are backwards incompatible changes that can slip through the automated checks. For instance, consider

Optional<Article> ArticleRepo::findById(Long articleId) {
    // New code
    if (articleId > Integer.MAX_VALUE) throw new IllegalArgumentException();

}

or less obvious

Optional<Article> ArticleRepo::findById(Long articleId) {
    // New code
    if (articleId > Integer.MAX_VALUE) return Optional.empty();

}    

The more general solution is to stop making backwards incompatible changes to your contracts. Instead, introduce new contracts and deprecate the old ones.