A (crazy?) approach to unit testing in Go

I truly value unit tests. They ensure our code is working as expected. They ideally can run in isolation instead of requiring connectivity to external services, which is a good thing. Unit tests can also help identify bug and ensure regressions don’t occur. This is much less “expensive” than if a bug or regression makes it all the way to a release.

But if there’s one thing I really dislike about writing unit tests, especially in Go, is that they’re typically a pain to write! And I frequently find it even harder to modify an existing unit test when I need to change the behavior of some function.

Let’s take a look at some pseudocode from Heptio Ark, the Kubernetes backup and recovery tool that I work on. This is from the ItemBackupper, whose sole purpose is (you guessed it!) to back up a single Kubernetes resource. The function in question is called backupItem.

1. Check if we should skip backing up the item because its namespace is excluded
2. Check if we should skip backing up the item because the user doesn’t want to back up cluster-scoped resources
3. Check if we should skip backing up the item because its resource (e.g. events) is in the user’s excluded resources list
4. Check if we should skip backing up the item because we’ve already backed it up
5. Run pre hooks for this item, if any
6. For each item action plugin
  a. Check if the plugin is applicable based on resource
  b. Check if the plugin is applicable based on namespace
  c. Check if the plugin is applicable based on label selector
  d. Execute action
  e. Back up all additional items indicated by the action (if any)
7. If the item is a PV, try to snapshot it
8. Run post hooks for this item, if any
9. Write item json using the tar file writer

There are a lot of things happening there. It’s close to a nightmare to test. We have to mock or stub certain interfaces. As the number of test cases grows, we have to add more and more conditional logic into the test case code itself to make sure the unit tests don’t have nil pointer issues, etc.

I’ve wondered if we could somehow break this up into a smaller unit of work, but this really is about as small as it gets. We could perhaps take the initial set of checks and extract them into a single shouldBackup() function, and maybe we could do similar things to other bits and pieces, but at the end of the day, the act of backing up a single item can’t get any smaller.

Let’s consider extracting steps 1–4 into a separate shouldBackup() function. We could fairly easily unit test that function by itself. But I would still want to know that all the test cases in TestShouldBackup() are also tested by TestBackupItem(). To state this another way, I want to be certain that backupItem() calls shouldBackup(), and I don’t want to have to replicate all the test cases for shouldBackup() in both TestShouldBackup() and TestBackupItem(). Yet I feel like if I only test a single path for shouldBackup() inside TestBackupItem(), I can’t be 100% certain that backupItem() will continue to handle all of the “should I back this up?” checks it needs to.

For example, what if someone changed backupItem() so that instead of calling shouldBackup(), it now directly handles the 1 “should I back this up” check that we have in TestBackupItem()? That test will still pass. As will TestShouldBackup(). But in this scenario, backupItem() has now regressed.

And now for my possibly crazy suggestion

I remembered looking at the Packer project’s source code a while ago, and that it has the concept of “steps.” For example, when building a virtual machine image, its Parallels ISO Builder has a series of steps that are executed in the order they’re specified: prepare Parallels tools, download the ISO, set up the output directory, …, create a VM, …, launch the VM, and so on.

That got me to thinking: maybe this could address my concerns. What if we did something like this?

// file: pkg/steps/step.go (or something like that)
type Step interface {
  ValidateInputs(data map[string]interface{}) error
  Execute(data map[string]interface{}) (bool, error)
}
type Executor struct {
  Steps []Step
}
func NewExecutor(steps ...Step) *Executor { ... }
func (e *Executor) Execute(data map[string]interface{}) error {
  for _, s := range e.Steps {
    if err := s.ValidateInputs(data); err != nil {
      return err
    }
    keepGoing, err := s.Execute(data)
    if err != nil {
      return err
    }
    if !keepGoing {
      return nil
    }
  }
  return nil
}

We have a fairly basic multi-step executor. Each Step is able to validate its expected set of inputs, and assuming that succeeds, execute some logic. I’m using a simple map[string]interface{} to pass data from one step to the next, but this could be a more complex struct with additional fields such as a logger, and/or something based on Go’s context.Context. Finally, each step can indicate if the executor should proceed to the next step, or short-circuit and stop.

Here is what it might look like in use:

// file: pkg/backup/item_backupper.go
type stepShouldIncludeNamespace struct{}
func (s *stepShouldIncludeNamespace) ValidateInputs(data map[string]interface{}) error {
  // ...
}
func (s *stepShouldIncludeNamespace) Execute(data map[string]interface{}) (bool, error) {
  item := data["item"].(runtime.Unstructured)
  metadata, err := meta.Accessor(obj)
  // check err
  namespace := metadata.GetNamespace()
  namespaces := data["namespaces"].(*collections.IncludesExcludes)
  if namespace != "" && !namespaces.ShouldInclude(namespace) {
    // false in this case means short-circuit the step executor
    return false, nil
  }
  // true in this case means move on to the next step
  return true, nil
}
func (ib *defaultItemBackupper) newStepExecutor() *steps.Executor {
  return steps.NewExecutor(
    &stepShouldIncludeNamespace{},
    // add more steps here
  )
}
func (ib *defaultItemBackupper) backupItem(item runtime.Unstructured) error {
  stepData := make(map[string]interface{})
  stepData["item"] = item
  stepData["namespaces"] = ib.namespaces
  executor := ib.newStepExecutor()
  return executor.Execute(stepData)
}

If we do something like this, we can directly address my original concern, where we have adequate unit test coverage (line coverage, that is) but in reality there’s a bug in backupItem(): we write a test case to ensure all of the steps are included, in the proper order:

func TestNewStepExecutor(t *testing.T) {
  ib := setupItemBackupper()
  e := ib.newStepExecutor()
  assert.Len(e.Steps, 1)
  assert.Equal(t, e.Steps[0], &stepShouldIncludeNamespace{})
}

This isn’t 100% foolproof, but it does make me feel a bit better about both writing testable code as well as modifying existing code and making it easy to adjust the tests accordingly.

I’d love to hear your thoughts. Is this crazy? Do you like it? Do you have other suggestions? Please share you ideas — thanks!