Encoding State Requirements in Types

In my previous post I looked at a simple way to eliminate conditional expressions by encoding them into the Scala type system. In this follow-up post I want to look at taking this further by using the Scala type system to encode state rules. The aim, as always, is to create code that fails to compile rather than code that fails at runtime. By doing this we also reduce the number of unit tests required.

The Problem

Consider a simple shopping workflow. I collect information about a Basket, the Customer and their Payment Method. Once I have all this in place I can create a processOrder function that completes the workflow process. As a naive starting point, lets encode this as a simple domain model holding optional values:

  case class CheckoutWorkflow(basket: Option[Basket], 
                              customer: Option[Customer], 
                              paymentMethod: Option[PaymentMethod])

  object CheckoutWorkflow {
    def empty = new CheckoutWorkflow(None, None, None)
  }                             

Then we need some functions that populate the workflow:

  def processBasket(workflow: CheckoutWorkflow): CheckoutWorkflow = {
    if ( workflow.basket.isDefined )
      throw new IllegalStateException("Basket workflow step already processed")

    // Do some processing...
    workflow.copy(basket = Some(basket))
  }

  def processCustomer(workflow: CheckoutWorkflow): CheckoutWorkflow = {
    if ( workflow.customer.isDefined )
      throw new IllegalStateException("Customer workflow step already processed")

    // Do some processing...
    workflow.copy(customer = Some(customer))
  }

  def processPaymentMethod(workflow: CheckoutWorkflow): CheckoutWorkflow = {
    if ( workflow.paymentMethod.isDefined )
      throw new IllegalStateException("Payment Method workflow step already processed")

    // Do some processing...
    workflow.copy(paymentMethod = Some(paymentMethod))
  }

Note that each of the above functions has a guard condition to stop them being called multiple times for the same workflow. Each of these guard conditions would require a separate unit test to ensure that it works and to avoid regressions should it be accidentally removed in the future.

Finally, we need the method to process the order. Given our domain model above, this class needs to contain some conditional check to ensure that all the workflow requirements are satisfied before processing of the order can commence. Something like:

  def processOrder(workflow: CheckoutWorkflow) = workflow match {
    case CheckoutWorkflow(Some(basket), Some(customer), Some(paymentMethod)) => {
      // Do the order processing
    }
    case _ => throw new IllegalStateException("Workflow requirements not satisfied")
  }

None of the above is obviously ideal as there are a number of places that have the potential to error at runtime. The conditionals pollute our code with non-business logic. Also, we have to write good unit tests to ensure all the conditionals are working correctly and have not been accidentally removed. Even then, any client may call our code having not met the requirements encoded in the conditionals and they will receive a runtime error. Let's hope they unit test as thoroughly as we do! Surely we can do better than this?

A Less Than Ideal Solution

Well, we could use the approach outlined in my previous post and use domain model extensions:

  case class WorkflowWithBasket(basket: Basket)
  case class WorkflowWithBasketAndCustomer(basket: Basket, customer: Customer)
  case class WorkflowWithAllRequirements(basket: Basket, 
                                         customer: Customer, 
                                         paymentMethod: PaymentMethod)

  def processOrder(workflow: WorkflowWithAllRequirements) = {
     // Do the order processing
  }                                         

While this does allow removal of all the conditionals and associated tests, it unfortunatley also reduces the flexibility of our model quite significantly in that the order that the workflow must be processed is now encoded into the domain model. Not ideal. We'd like to keep the flexibility from the first solution but in a type safe way. Is there a way that we can encode the requirements into the type system?

Levaraging The Type System

First, lets consider what we want to achieve. Our aim to encode unsatisfied and satisfied workflow requirements and only allow methods to be called when the correct combinations are set. So, let's first encode the concept of requirements:

  trait BasketRequirement
  case object UnsatisfiedBasketRequirement extends BasketRequirement
  
  trait CustomerRequirement
  case object UnsatisfiedCustomerRequirement extends CustomerRequirement
  
  trait PaymentMethodRequirement
  case object UnsatisfiedPaymentMethodRequirements extends PaymentMethodRequirement

Here we have defined requirement traits for each of the different workflow stages. We also defined case objects to represent the unsatisfied state of each requirement. Next we need to indicate the satisfied states, which are our actual domain object classes:

  case class Basket(items: List[LineItem]) extends BasketRequirement
  case class Customer(id: String) extends CustomerRequirement
  case class PaymentMethod(paymentType: PaymentType) extends PaymentMethodRequirement

Next job is to make sure that our workflow object can represent these requirements and be strongly typed on either the satisfied or unsatisfied state. We do this by adding type bounds to each of the workflow types. This also allows us to eliminate the need for the Option types. We also define an 'unsatisfied' instance as the starting point for our workflow:

  case class CheckoutWorkflow[B <: BasketRequirement,
                              C <: CustomerRequirement,
                              PM <: PaymentMethodRequirement]
                              (basket: B, customer: C, paymentMethod: PM)

  object CheckoutWorkflow {
    val unsatisfied = CheckoutWorkflow(UnsatisfiedBasketRequirement,
                                       UnsatisfiedCustomerRequirement,
                                       UnsatisfiedPaymentMethodRequirements)
  }

Now we need the functions that actually process each individual workflow stage. Note how each one defines type bounded parameters for the things it doesn't care about. However, for the stage that it actually manipulates it requires that it is called with the Unsatisfied type and returns the Satisfied type. Thus, you can no longer call any of these methods with a workflow that already has that stage satisfied: the compiler won't allow it:

  def processBasket[C <: CustomerRequirement, PM <: PaymentMethodRequirement]
    (workflow: CheckoutWorkflow[UnsatisfiedBasketRequirement.type, C, PM]): 
      CheckoutWorkflow[Basket, C, PM] = {

    // Do some processing...
    workflow.copy(basket = basket)
  }

  def processCustomer[B <: BasketRequirement, PM <: PaymentMethodRequirement]
    (workflow: CheckoutWorkflow[B, UnsatisfiedCustomerRequirement.type, PM]): 
      CheckoutWorkflow[B, Customer, PM] = {

    // Do some processing...
    workflow.copy(customer = customer)
  }

  def processPaymentMethod[B <: BasketRequirement, C <: CustomerRequirement]
    (workflow: CheckoutWorkflow[B, C, UnsatisfiedPaymentMethodRequirements.type]): 
      CheckoutWorkflow[B, C, PaymentMethod] = {

    // Do some processing...
    workflow.copy(paymentMethod = paymentMethod)
  }

Finally, our processOrder function becomes super simple. You just can't call it any more unless it has no Unsatisfied types:

  def processOrder(workflow: CheckoutWorkflow[Basket, Customer, PaymentMethod]) = {
    // Process the order
  }

One observation that was made to me by someone (a Java dev) who looked at my initial draft of the code was that the final solution looks more complex due to all the type bounds and that there's actually more classes due to the need to define the requirement traits and the Unsatisfied state objects. However, don't forget that this solution eliminates at least four conditional blocks and associated unit tests, simplifies others and also possibly reduces the number of tests that clients need to write on their code as well. Also, there's no possibility of runtime failure. If the code compiles then we have a much higher confidence that it will work. Well worth a tiny bit more type complexity in my book.

Unit Testing with MongoDB

Back in June I presented at the London Scala User Group (video here) on our experiences on a recent Scala project that used MongoDB.

One of the questions I was asked was how we go about mocking MongoDB for testing purposes (especially unit testing)? My answer was that we don’t! This raised a couple of eyebrows in the audience, but we’ve found this works just fine for us.

In this blog post I will explore our thoughts and reasoning in a bit more detail.

How do we test with MongoDB?

I’ll be honest: for the majority of our tests (even the unit ones) we just run against a local MongoDB instance. When we are testing repository classes we write tests against a real repository wired up against a real MongoDB instance. We assert against the contents in the MongoDB collections. In some cases, we even test service classes or even controllers with a live repository backend that writes to and retrieves from a real MongoDB instance.

“But you can’t do that!”, I hear you cry. Traditional thinking says that writing tests directly against the database will be too slow. We’ve actually found MongoDB (especially when running locally) to be so blindingly fast that we don’t need to consider doing anything else. For most updates and queries we tend to get sub-millisecond responses.

It’s also a hell of a lot easier and less fragile to back tests with real repository instances than it is to create and configure a load of mocks or strange database testing frameworks.

I’m pragmatic. If using a real database backend was making our tests run too slow or causing them to be too complex then I’d happily consider alternatives. Our experience with MongoDB is that it is both fast enough and that it’s query syntax is succinct enough that we don’t need to bother with those alternatives.

Do we use mocks at all for the database then?

Actually, I rarely use mocks in any of my Scala project now days. I’ve switched much more towards a style of isolating my calls to dependencies and then using mixin traits to override specific behavior for testing. (I’ll write a blog post about this approach soon).

From time to time we do need to write tests that make a repository dependency exhibit a specific behavior which is difficult to emulate against an actual database. In these cases we just use a custom test trait mixed into the repository in order to override methods to exhibit the behavior that we require. Keeps it simple and makes the tests much more readable and much less fragile than using mocks or other frameworks.

Concise, Elegant and Readable Unit Tests

Just recently I was pair programming with another developer and we were writing some unit tests for the code we were about to write (yes, we were doing real TDD!). During this exercise we came to a point where I didn't feel that the changes being made to the code met my measure of what makes a good test. As in all the best teams we had a quick discussion and decided to leave the code as it was (I save my real objections for times when it is more valuable). However, this got me thinking about what actually I was uncomfortable with, and hence the content of this post.

As a rule I like my code to have three major facets. I like it to be concise: there should be no more code than necessary and that code should be clear and understandable with minimum contextual information required to read it. It should be elegant: the code should solve the problem at hand in the cleanest and most idiomatic way. Also, it must be readable: clearly laid out, using appropriate names, following the style and conventions of the language it is written in and avoiding excessive complexity.

Now, the test in question was written in Groovy, but it could just as well be in any language as it was testing a fairly simple class that maps error information into a map that will be converted to Json. The test is:

@Test
void shouldGenerateAMapOfErrorInformation() {
    String item = "TestField"
    String message = "There was an error with the test field"
    Exception cause = new IllegalStateException()

    ErrorInfo info = new ErrorInfo()
    info.addError(item, message, cause)

    Map content = info.toJsonMap()
    assert content.errors.size() == 1
    assert content.errors[0].item == item
    assert content.errors[0].message == message
    assert content.errors[0].cause == cause.toString()
}

Now, to my mind this test is at least three lines too long - the three lines declaring variables that are then used to seed the test and in the asserts. These break my measure of conciseness by being unnecessary and adding to the amount of context needed in order to follow the test case. Also, the assertions of the map contents are less than elegant (and also not very concise), which further reduces the readability of the code. I'd much rather see this test case look more like this:

@Test
void shouldGenerateAMapOfErrorInformation() {
    ErrorInfo = new ErrorInfo()
    info.add("TestField", "There was an error with the test field", 
                 new IllegalStateException())

    Map content = info.toJsonMap()
    assert content.errors.size() == 1
    assert hasMatchingElements(contents.errors[0], 
          [ item : "TestField",
             message : "There was an error with the test field",
             cause : "class java.lang.IllegalStateException" ])
}

This test implementation is more concise. There are less lines of code and less context to keep track of. To my eye I find this much more elegant and far more readable.

"But, you've added duplication of the values used in the test", I hear you cry! That is true and it was totally intentional. Let me explain why…

Firstly, as we have already mentioned, duplicating some test values actually makes the code more concise and readable. You can look at either the test setup/execution or the assertions in isolation without needing any other context. Generally, I would agree that avoiding duplication in code is a good idea, but in cases such at this, the clarity and readability far out way the duplication of a couple of values!

Secondly, I like my tests to be very explicit about what they are expecting the result to be. For example, in the original test, we asserted that the cause string returned was the same as the result of calling toString() on the cause exception object. However, what would happen if the underlying implementation of that toString() method changed. Suddenly my code would be returning different Json and my tests would know nothing about it. This might event break my clients - not a good situation.

Thirdly, I like the intention of my code to be very clear, especially so in my tests. For example, I might create some specific, but strange, test data value to exercise a certain edge case. If I just assigned it to a variable, then it would be very easy for someone to come along, think it was an error and correct it. However, if the value was explicitly used to both configure/run the code and in the assertion then I would hope another developer might think more carefully as to my intent before making a correction in multiple places.

My final, and possibly most important reason for not using variables to both run the test and assert against is that this can mask errors. Consider the following test case:

@Test
void shouldCalculateBasketTotals() {
    Basket basket = new Basket()
    LineItem item1 = new LineItem("Some item", Price(23.88))
    LineItem item2 = new LineItem("Another item", Price(46.78))
 
    basket.add(item1)
    basket.add(item2)
 
    assert basket.vatExclusivePrice == 
                       item1.vatExclusivePrice + item2.vatExclusivePrice
    assert basket.vat == item1.vat + item2.vat
    assert basket.totalPrice == item1.price + item2.price
}

In this test, what would happen if the LineItem class had an unnoticed rounding error in its VAT calculation that was then replicated into the basket code? As we are asserting the basket values based on the fixture test data we may never notice as both sides of the assertion would report the same incorrectly rounded value. By being more explicit in our test code we not only create code that is more concise, elegant and readable but we also find more errors:

@Test
void shouldCalculateBasketTotals() {
    Basket basket = new Basket()
    LineItem item1 = new LineItem("Some item", Price(23.88))
    LineItem item2 = new LineItem("Another item", Price(46.78))
 
    basket.add(item1)
    basket.add(item2)
 
    assert basket.vatExclusivePrice == 58.88
    assert basket.vat == 11.78
    assert basket.totalPrice == 70.66
}

Finally, a couple more points that people might raise and my answers to them (just for completeness!):

Q: Can't we move the constant variables up to the class level and initialise them in a setup to keep the tests methods concise?
A: This makes an individual method contain less lines of code but fails my consiseness test as there is context outside of the test method that is required to understand what it does. You actually have to go and look elsewhere to find out what values your test is executing against. This is also less readable!

Q: Isn't is better to share test fixtures across test methods in order to keep the test class as a whole more concise?
A: This might be appropriate for some facets, such as any mocks that are used in the test or if there is a particularly large data set required for the test which is not part of the direct subject of the test. However, I'd rather see slightly longer individual test methods that are explicit in their test data and assertions, even if this means some duplication or a slightly longer overall test class.