Reviewing tests to uncover design problems

Most of us learn to write tests at work by imitating existing tests. It is both a pro and a con. Pro, because we get to learn a new skill at work. Con, because often we may not fully understand the design decisions that went into the test and corresponding production code or vice-versa.

As usual, let's begin with a simple code snippet. The following test serves as a starting point for ideas you can use to spot design flaws in your test, production code, and maybe even your testing strategy. Take some time to grasp the code because we will be iterating on it throughout this article.

@Test
fun `repository can insert recipe into the database`() {
  // given
  val recipeDao = mock<RecipeDao>()
  val recipeRepository = RecipeRepository(recipeDao)

  val bundle = mock<Bundle>()
  whenever(bundle.getString(KEY_RECIPE_ID, ""))
    .thenReturn("123e4567-e89b-42d3-a456-556642440000")
  whenever(bundle.getString(KEY_RECIPE_TITLE, ""))
    .thenReturn("Party Coffee Cake")
  whenever(bundle.getString(KEY_RECIPE_DESCRIPTION, ""))
    .thenReturn("Perfect coffee cake for parties.")
  whenever(bundle.getString(KEY_RECIPE_IMAGE_URL, ""))
    .thenReturn("https://example.com/coffee-cake.jpg")
  whenever(bundle.getInt(KEY_RECIPE_CALORIES, 0))
    .thenReturn(270)

  // when
  recipeRepository.save(bundle)

  // then
  verify(recipeDao).insert(any<Recipe>())
}

The motive behind this test is to verify if  RecipeRepository can save a Recipe into the database.

Zooming out

First, let's take a high-level view of the snippet.

1. It is a unit test written for an Android app, and it runs on the JVM (we infer this from the source set).
2. The file's name is RecipeRepositoryTest.kt, and the class RecipeRepositoryTest declares the test.
3. The name of the test is repository can insert recipe into the database.

Now that we have looked at the high-level information, let's zoom in and look into finer details.

Zooming in

The test uses four types that are of interest.

1. RecipeDao, an abstraction representing operations on the database.
2. The RecipeRepository is the system under test (a.k.a. SUT).
3. The Android Bundle. It is a mechanism Android uses to transfer data during both inter and intra process communication. It is worthy to note this is a platform type and not part of our business domain.
4. Recipe model holds the properties of a recipe.

The RecipeRepository is a real object, whereas RecipeDao and Android's Bundle are mocked objects.

As we proceed, I want to review this test in two different categories — the first one in terms of API design and the second one in terms of testing strategy.

1. Reviewing API design

Reviewing tests from the viewpoint of API design helps us improve readability, maintainability, and ease of consuming the production code.

1.1 Design smell: Coupling

As developers, we often have instincts. You know, the feeling when something feels wrong, but you can't find words to explain why. If we learn to pay attention to these instincts instead of brushing them off, we'll often derive meaningful insights. If a piece of code feels painful or frustrating to type in, more often than not, it could be an underlying design problem. So, it's worth paying attention to your instincts and feelings when you read or write code.

From our sample test, you may already feel uncomfortable looking at lines 7 through 17, and that's what we will discuss first. Let me replicate the lines under discussion for your convenience.

  // …
  val recipeRepository = RecipeRepository(recipeDao)

  val bundle = mock<Bundle>()
  whenever(bundle.getString(KEY_RECIPE_ID, ""))
    .thenReturn("123e4567-e89b-42d3-a456-556642440000")
  whenever(bundle.getString(KEY_RECIPE_TITLE, ""))
    .thenReturn("Party Coffee Cake")
  whenever(bundle.getString(KEY_RECIPE_DESCRIPTION, ""))
    .thenReturn("Perfect coffee cake for parties.")
  whenever(bundle.getString(KEY_RECIPE_IMAGE_URL, ""))
    .thenReturn("https://example.com/coffee-cake.jpg")
  whenever(bundle.getInt(KEY_RECIPE_CALORIES, 0))
    .thenReturn(270)

  // when
  recipeRepository.save(bundle)

  // … 🤷🏽‍♂️

On line 7, we have a mock Bundle object. From lines 8 through 17, we are stubbing method calls on the object to return various values.

For instance, if we call the method getString on the mocked bundle instance with KEY_RECIPE_TITLE and an empty string "" as arguments, the method will return the String "Party Coffee Cake" (lines 10-11 ).

On line 20, we pass the stubbed bundle object to the repository itself. For this test to work, the RecipeRepository's save method implementation should call the stubbed methods with the exact arguments on the bundle instance. If you break open the save method inside RecipeRepository, you wouldn't be surprised if the method contains some variation of the following speculative implementation.

class RecipeRepository(private val recipeDao: RecipeDao) {
  fun save(bundle: Bundle) {
    val id = bundle.getString(KEY_RECIPE_ID, "")
    val title = bundle.getString(KEY_RECIPE_TITLE, "")
    val description = bundle.getString(KEY_RECIPE_DESCRIPTION, "")
    val imageUrl = bundle.getString(KEY_RECIPE_IMAGE_URL, "")
    val calories = bundle.getInt(KEY_RECIPE_CALORIES, 0)

    // more code…
  }
}

The stubbing setup in the test mimics the production code (lines 3 to 7). What seems like near duplication on the surface is a much more severe problem. The test code knows the implementation details of your production code; this means there is a tight coupling between them. Let's see what the implications are. Imagine the following scenarios,

1. Your team decides to use -1 as a default value for KEY_RECIPE_CALORIES instead of 0.
2. Your content team is now making videos for these recipes and wants to include a video URL attribute.

In both scenarios, changing the production code will make the test(s) fail. To prevent this from happening, we have to make sure all the stubs are in sync with the production code, which means updating the default value for calories and adding new stubs for the video URL property. Any changes to the production code will cause the tests to fail and vice-versa. And if you aren't familiar with this situation, we call it the fragile test problem.

The root cause for fragility in this situation is exposing the implementation details. Exposing implementation details to tests or any caller causes tight coupling, and that in turn causes fragility.

1.2 Guideline violation: Functions should do one thing

The RecipeRepository.save(Bundle) function does more than one thing. First, it extracts the recipe's attributes from the Bundle, and second, it stores them into the database. Though this is a minor violation compared to coupling, this may cause code duplication and require us to perform a shotgun surgery if the recipe attributes we put inside the Bundle change.

1.3 Design smell: Platform classes in the domain layer

We know Bundle is an Android platform class; we also know RecipeRepository is a domain class. Referencing a platform class from the domain causes coupling between the two, which may affect the following.

1. Testability - Depending on the amount of platform class usage, you may have to implement reliable fakes, use mocks (risky and less reliable), or run the tests inside the platform's environment (maybe slow but required). Mocking classes we don't own can be risky, and we should only use them with justification. This situation does not warrant mocking a class we don't own (i.e.) the Bundle.
2. Portability - Though this is not the goal, it is a litmus test to see how decoupled the domain code is from the platform. If we were to reuse this code in an iOS application, the Bundle dependency would prevent direct reuse.
3. Readability & maintainability - Platform classes are designed to cater to many cases and therefore are very generic. Having a platform class in the domain layer may not make its relationship with the domain object apparent. Consequently, we must dig deeper into the code to understand the relationship between domain and platform classes.

2. API design fixes

We uncovered the following problems in the previous section.

1. Test fragility as a side-effect of tight coupling between test and production code.
2. The RecipeRepository.save(Bundle) function does more than one thing.
3. Android platform classes polluting the domain layer, thereby affecting testability, portability, and maintainability.

Surprisingly, if we decouple the Bundle platform class from the domain layer, it will fix all three problems like a domino effect.

2.1 Making the relationship between Recipe and Bundle explicit

We know that the repository's save function extracts Recipe attributes from the Bundle. The platform class acts as a carrier of information required by the domain class, and this is how the platform and domain class are related to one other. Our first step is to extract this logic and put it in a separate place so that it is easier for us to test and modify this relationship as it evolves.

Let's say we have a Recipe domain class that looks like the one shown below.

data class Recipe(
  val id: String,
  val title: String,
  val description: String,
  val imageUrl: String,
  val calories: Int
)

We have two things that belong to the platform — one, the logic to extract the recipe's attributes from the Bundle and two, the key constants for those attributes. It could be tempting to add both to the Recipe class by creating a Kotlin companion object. That attempt is just shifting the problem from RecipeRepository to Recipe, not solving it. What we could do instead is add an empty companion object to Recipe.

data class Recipe(
  val id: String,
  val title: String,
  val description: String,
  val imageUrl: String,
  val calories: Int
) {
  companion object
}

If you look at line 8, it may not seem much, but what this effort unlocks for us is the ability to create extension functions for the companion object. Which in turn helps us isolate platform-specific functionality and keep platform classes away from the domain layer.

Now, let's create a new Kotlin file and isolate platform-specific data and functionality.

// Extension properties
internal val Recipe.Companion.KEY_RECIPE_ID: String
  get() = "recipe_id"
internal val Recipe.Companion.KEY_RECIPE_TITLE: String
  get() = "recipe_title"
internal val Recipe.Companion.KEY_RECIPE_DESCRIPTION: String
  get() = "recipe_description"
internal val Recipe.Companion.KEY_RECIPE_IMAGE_URL: String
  get() = "recipe_image_url"
internal val Recipe.Companion.KEY_RECIPE_CALORIES: String
  get() = "recipe_calories"

// Extension function
internal fun Recipe.Companion.from(bundle: Bundle): Recipe {
  val id = bundle.getString(KEY_RECIPE_ID, "")
  val title = bundle.getString(KEY_RECIPE_TITLE, "")
  val description = bundle.getString(KEY_RECIPE_DESCRIPTION, "")
  val imageUrl = bundle.getString(KEY_RECIPE_IMAGE_URL, "")
  val calories = bundle.getInt(KEY_RECIPE_CALORIES, 0)

  return Recipe(id, title, description, imageUrl, calories)
}

With RecipeAndroidExtensions.kt, we have concentrated all knowledge about the relationship between the platform Bundle class and the Recipe domain class in one place.

P.S. You can still sense the stink from using default sentinel values like "" and 0, but we won't discuss it here in this article.

2.2 Decoupling RecipeRepository from Bundle

RecipeRepository belongs to the domain layer but still references Bundle. Since we have isolated platform-specific logic to a separate Kotlin file, now for functions like save that require a Bundle parameter, we can pass a Recipe object instead.

There are a couple of different ways to make this change safely. You can use IntelliJ's change signature refactor action, or you can use Kotlin's @Deprecated annotation along with IntelliJ's tooling support to make this change.

Let's change the signature of the save function from this,

class RecipeRepository(private val dao: RecipeDao) {
  fun save(bundle: Bundle) {
    // …
  }
}

to that.

class RecipeRepository(private val dao: RecipeDao) {
  fun save(recipe: Recipe) {
    // …
  }
}

With these changes, we can update our tests and get rid of the stubbing altogether.

@Test
fun `repository can insert recipe into the database`() {
  // given
  val recipeDao = mock<RecipeDao>()
  val recipeRepository = RecipeRepository(recipeDao)

  val recipe = Recipe(
    id = "123e4567-e89b-42d3-a456-556642440000"
    title = "Party Coffee Cake",
    description = "Perfect coffee cake for parties.",
    imageUrl = "https://example.com/coffee-cake.jpg",
    calories = 270
  )

  // when
  recipeRepository.save(recipe)

  // then
  verify(recipeDao).insert(any<Recipe>())
}

Lines 7 - 13 now create a real Recipe object instead of a stubbed Bundle, and line 16 takes in a Recipe object instead of a Bundle. Therefore, effectively decoupling the domain from the platform.

With these changes in place, we have resolved the following problems identified during our API design review.

• Remove tight coupling between test and production code, thereby addressing test fragility.
• Ensure the save function does one thing only.
• Decouple domain classes from the platform, which in turn improved testability, portability, and maintainability.

3. Reviewing test strategy

Reviewing the API design can help us identify and fix design problems related to the system and the programming paradigm. However, reviewing the test strategy can help us improve our confidence in the test(s). Whenever you review tests, ask two questions — "What are we testing?" and "How are we testing it?"

@Test
fun `repository can insert recipe into the database`() {
  // given
  val recipeDao = mock<RecipeDao>()
  val recipeRepository = RecipeRepository(recipeDao)

  val recipe = Recipe(
    id = "123e4567-e89b-42d3-a456-556642440000"
    title = "Party Coffee Cake",
    description = "Perfect coffee cake for parties.",
    imageUrl = "https://example.com/coffee-cake.jpg",
    calories = 270
  )

  // when
  recipeRepository.save(recipe)

  // then
  verify(recipeDao).insert(any<Recipe>())
}

3.1 What are we testing?

Looking at the test, we can clearly say that we are verifying if RecipeRepository can insert a Recipe into the database. I like the laser focus here and the fact that this test verifies one thing only.

3.2 How are we testing it?

This question is not tricky to answer but requires an elaborate explanation.

  // …

  // when
  recipeRepository.save(recipe)

  // then
  verify(recipeDao).insert(any<Recipe>())
}

First, let's talk about the assertion itself. Line 19 uses Mockito to verify if  recipeDao's  insert function was invoked with a Recipe object as an argument.

When we use argument matchers like any, which only checks the type of the argument, we are diluting our confidence in the assertion. There is no guarantee that the inserted recipe has the same properties as the one passed to the repository's save method.

One way to fix this is to ensure that all properties are the same by passing a structurally equivalent instance as the expected argument, thereby regaining some degree of confidence in the assertion.

The second factor that's slightly disturbing is the absence of a negative assertion. We need to ensure that save is not calling any more methods on the RecipeDao. We can fix this problem by introducing a negative assertion — a verifyNoMoreInteractions(recipeDao) statement after the existing verification call on line 19.

  // …

  // when
  recipeRepository.save(recipe)

  // then
  verify(recipeDao).insert(recipe)
  verifyNoMoreInteractions(recipeDao)
}

An improvement for both the problems discussed earlier would look like the changes we made to lines 19 and 20 in the snippet above. It is tempting to call it a day and move forward. But hold on, there is a serious issue with the test here.

When we ask how are we testing "it"? The "it" refers to the repository inserting an entry into the database. Right now, the answer to this question is funny. The test states if the ReceipeRepository.save(Recipe) function calls the RecipeDao.insert(Recipe) function, a new entry was inserted into the database. This comically translates to — if a certain object sends a message to another object, we can be sure that there's an entry in the database.

The objective of writing tests is to verify assumptions; however, it's ironic that this test itself is making assumptions of its own and therefore undermines our confidence in it.

3.3 Trade-offs

Even though the intention behind this test is pure, this is one of the cases that demonstrates why good intentions alone are often not enough — the test trades-off reliability in exchange for speed and convenience. Trading off reliability for other attributes in testing almost always won't end well for its maintainers.

The current test is not the right kind of test for the concern we are interested in. The situation warrants an integration test that can work with a real database.

4. Test strategy fixes

1. Add deprecation notice to the RecipeRepositoryTest unit test class.
2. Create a new headless instrumented Android test that uses a real RecipeDao to test the RecipeRepository's database operations.
3. The RecipeRepositoryInstrumentedTest alone is sufficient. You don't have to write an integration test for RecipeDao. Structural covariance between tests and production code is unnecessary and is also undesired. Check this article from Uncle Bob for more details.
4. Verify insertions by querying the database; this ensures what you put into the database can also be retrieved without problems.
5. Write migration tests while making changes to the Recipe model. Now that we are dealing with a real database, we now look into aspects we didn't bother with before.
6. Once the integration test is in place, deleted the deprecated RecipeRepositoryTest unit test.

These steps should get you covered with the right kind of tests for RecipeRepository. An incorrect testing strategy will undermine confidence in your entire test suite. It can also be dangerous because engineers tend to pick patterns from the existing codebase and replicate them. If you have tests in your codebase but don't trust them, check if you have the right kind of tests before adding more tests.

Takeaways

1. Review tests in two parts — API design and test strategy.
2. Reviewing API design will help you uncover design problems in your system and programming paradigm.
3. Reviewing the test strategy can help you choose the right kind of tests and improve your confidence in them.
4. Pay attention to your instincts and feelings while reading or writing tests. If a test is particularly hard or not fun to work with, it may hint at an underlying design problem.
5. Exposing implementation details to tests can cause coupling and test fragility.
6. Polluting domain classes with platform classes will compromise testability, portability, and maintainability.
7. Empty companion objects in Kotlin domain classes can provide hooks to keep platform-specific classes and functionality at bay.
8. While reviewing a test strategy, ask two questions - "What are we testing?" and "How are we testing it?".
9. Don't shy away from writing integration tests that use an actual database, filesystem, etc., particularly if they improve the reliability of your tests.
10. If you have an extensive test suite but don't trust it enough, consider checking if you have the right kind of tests before adding more tests.

If you enjoyed this article, you might also like the little nuggets of knowledge I share more frequently on Twitter 🙂