Net Objectives

Net Objectives
If you are interested in coaching or training in ATDD or TDD please click here.

Friday, August 9, 2013

TDD Mark 3 Introduced

First of all, sorry for the long absence.  Our training schedule has been wall-to-wall, and when one of us had a brief gap the other has always been busy.

It has given us time to think, however.  Long airplane rides and such. :)

We're been playing around with an idea we're calling (for the moment) TDD Mark 3 (the notion that TDD is not about testing but rather about specification being TDD Mark 2).  To give you an idea of what we're thinking, let's look at an example of TDD Mark 2 as we've been writing tests up to this point, and then refactor it to the TDD Mark 3 style.

Mark 2

So, what is the requirement? Our client is a cruise ship operator. Some of the stuff on the cruise is free and the rest are paid extras. On the ship, the guest can pay for extras with a standard credit card, or with the ship's debit card. Paying with the ship's debit card gives the guest a 5% discount on the purchase cost. The catch is that if you want to use the ship's debit card the guest has to load the card for the first time with at least $2,000. If you try to load a card with less than that amount, the transaction should fail.

[TestClass]
public class FundLoaderTDD
{

  [TestMethod]
  public void TestMinimalFunds()
  {
    Assert.AreEqual(2000, Card.MINIMAL_FUNDS);
  }

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    
uint minimalFunds = Card.MINIMAL_FUNDS;        
    Card card = new Card(/*card holder's details*/);
    card.LoadFunds(minimalFunds);
    

    uint insufficientFunds = minimalFunds - 1;
    card = new Card(/**/);
    try
    {
      card.LoadFunds(
insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  }
}



The meaning here is very clear: the LoadFunds() method of Card will throw an InsufficientFundsException if you try to load an amount less than the minimal allowed value.  We also show that if the minimal amount is loaded, an exception is not thrown.  This constitutes a very typical specification of a behavioral boundary anchored at the value MINIMAL_FUNDS. Note also that we have specified what that value is in the first test.

Naturally, there are many other tests that specify the various aspects of the Card's behavior, and together they turn the user's requirement into an executable specification.  That's what Mark 2 is all about.

Refactor to Mark 3

We all know the importance of good design. Good design enables proper code maintainability (more on that in a future blog), which has to do with dealing with change.

We should also acknowledge that the tests that we write are not "second class citizens". They require as much love and attention as the production code they specify. This means that after the test has been written we have an opportunity to refactor its design. This is done with respect to specific changes that may be required in the code. These can come from two sources - changing requirements and changing the domain model to reflect changing responsibilities.

Changing requirement could comprise raising the minimal limit or creating a graded discount structure. Changing the domain comprises adding or removing classes or methods on classes.

The customers new requirement is this: there are other ways to charge the user for on-board services. It turns out that guests often do not carry the card with them (to the pool, for example) but would still like to purchase cute drinks with little pink umbrellas. To enable that, a biometric system was installed where the guest can charge the drink to his card by swiping their finger over a fingerprint scanner incorporated into the card reader held by the server.

This means that the model we created where the Card was the central object needs to be refined, and an Account class introduced. The Card is just one way if interacting with the account.

What affect will this have on our test? All reference to Card must be replaced with references to Account. Considering out test code there are two redundancies that we can identify: Card.MINIMAL_FUNDS and card.LoadFunds().

[TestClass]
public class FundLoaderTDD
{

  [TestMethod]
  public void TestMinimalFunds()
  {
    Assert.AreEqual(2000, Card.MINIMAL_FUNDS);
  }

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    

    uint minimalFunds = Card.MINIMAL_FUNDS;         
    Card card = new Card(/*Any card holder's details*/);
          card.LoadFunds(minimalFunds);
    

    uint insufficientFunds = minimalFunds - 1;
    card = new Card(/*Any card holder's details*/);
    try
    {
      card.LoadFunds(
insufficientFunds);
      Assert.Fail("Card should have thrown a " +

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  }
}


We don't like redundancies in our tests any more than we like them in our production code.  We extract the redundancies into methods:


[TestClass]
public class FundLoaderTDD
{

  [TestMethod]
  public void TestMinimalFunds()
  {
    Assert.AreEqual(2000, MinimalFunds());
  }

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {

    uint minimalFunds = MinimalFunds();      
    card = new Card(/*Any card holder's details*/);
    LoadFunds(minimalFunds);

    
    uint insufficientFunds = minimalFunds - 1;
    card = new Card(/*Any card holder's details*/);
    try
    {
      LoadFunds(
insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  } 


  Card card;
  private UInt MinimalFunds() 
  {
    return Card.MINIMAL_FUNDS;
  }

  private void LoadFunds(uint funds)
  {
    card.LoadFunds(funds);
  }
}


We can inline the MinimalFunds private function and get:

[TestClass]
public class FundLoaderTDD
{

  [TestMethod]
  public void TestMinimalFunds()
  {
    Assert.AreEqual(2000, MinimalFunds());
  }

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    card = new Card(/*
Any card holder's details*/);
    LoadFunds(MinimalFunds());
    

    uint insufficientFunds = MinimalFunds() - 1;
    card = new Card(/*Any card holder's details*/);
    try
    {
      LoadFunds(
insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  } 


  Card card;
  private UInt MinimalFunds() 
  {
    return Card.MINIMAL_FUNDS;
  }

  private void LoadFunds(uint funds)
  {
    card.LoadFunds(funds);
  }
}


Wait! There's another redundancy above:

    try
    {
     //...

      Assert.Fail("Card should have thrown a " +
 

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      //...

    }


We are specifying the type of the exception twice...We'll deal with that redundancy in a bit, so we'll put it on the to-do list. In the meanwhile, back to the refactored tests.  I do not like the name we gave the LoadFunds method, it's misleading. The customer not want the exception to be thrown every time the card is loaded with a small amount -- only on the initial load. So perhaps this is better:

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    
LoadInitialFunds(MinimalFunds());
    

    uint insufficientFunds = MinimalFunds() - 1;
    try
    {
     
LoadInitialFunds(insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

                 typeof(Card.InsufficientFundsException).Name());
    }
    catch (Card.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  } 



  Card card;
  private void LoadInitialFunds(uint funds)
  {
    card = new Card(/*Any card holder's details*/);
    card.LoadFunds(funds);
  }

Note the fact that card's initialization was moved into the LoadInitialFunds method.


Besides shifting the funds handling responsibility to the Account object, it was also deemed useful to shift the initial amount loading from a specific method to the constructor. So for the $64,000 question - how many places do we need to make this change in? One:

  Card card;
  private void  LoadInitialFunds(uint funds)
  {
    card = new Card(/*card holder's details*/);
    Account account = new Account(funds);
  }

And where should the limit be defines? Account, and it will return the value in a method.
  
  private UInt MinimalFunds() 
  {
    return Account.MinimalFunds();
  }

Finally, we can make the changes in the test, but only because we left the two references to the exception in the test.

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    
    LoadInitialFunds(MinimalFunds());
    

    uint insufficientFunds = MinimalFunds() - 1;
    try
    {
     
LoadInitialFunds(insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

              typeof(Account.InsufficientFundsException).Name());
    }
    catch (Account.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  }



The public methods are the specification, the private methods encapsulate implementation. Well, almost, with the exception of the exception handling. But why is an exception being thrown at all?

Well, if you remember, the customer wanted the user to be notified if the amount is too small. Exceptions are just one way of doing it. So we can safely say that the specific exception is an implementation detail, and based on the role we want the public method to play - specification, we really need to get that implementation detail out of here.

So, here's a question to our readers. How would you do it? Note that although we used C# right now, the refactoring principles are relevant to any language.

So without dealing with the exception, yet, this is what the test code looks like.

[TestClass]
public class FundLoaderTDD
{

  [TestMethod]
  public void TestMinimalFunds()
  {
    Assert.AreEqual(2000, MinimalFunds());
  }

  [TestMethod]
  public void TestLoadingLessThanMinimalFundsThrowsException()
  {
    LoadInitialFunds(
MinimalFunds());
    

    uint insufficientFunds = MinimalFunds() - 1;
    try
    {
     
LoadInitialFunds(insufficientFunds);
      Assert.Fail("Card should have thrown a " +
 

              typeof(Account.InsufficientFundsException).Name());
    }
    catch (Account.InsufficientFundsException exception)
    {
      Assert.AreEqual(
insufficientFunds, exception.Funds());
    }
  } 


  private UInt MinimalFunds()   {
    return Account.MINIMAL_FUNDS;
  }

  private void LoadFunds(uint funds)
  {
    Account account = new Account(funds);
  }
}


The public methods now essentially constitute an acceptance test.  In fact, those familiar with acceptance testing frameworks like FIT would express what these unit test methods communicate in another form, like a table for example, and the private methods would be the fixtures written to connect the tests to the system's implementation.

This does make the test class longer and more verbose, but it also makes it easier to read just the specification part, if that's all you are interested in.  Also, when design changes are made later (lets say, for example, that we decide to build the Account in a factory, or store the minimal initial value in a configuration file) that only one private method will be effected by a given change, and none of the public methods at all (which makes sense, since the design has been altered but not the acceptance criteria).

Mark 3

The separation in perspectives that was created in the above code is a result of refactoring. But it actually makes sense regardless. The public test method is written by intention,and describes the conceptual behavior of the system.

We also have a separation between the specification and implementation. We call these - different perspectives. And they allow us to focus on getting the requirement right, and then getting the design right. We can change the design without affective the requirement.

This is a major piece of making TDD sustainable. As this allows us to change the system design without affecting the public tests which specify the behavior.

So, the $1.000,0000 question is: "Why not write the tests that way to begin with?"

To Be Continued....

9 comments:

  1. Now to the main comment. I think I get what you're trying to do. As I see your intention with Mark 3, the goals are to:
    1. Add expressiveness to the tests
    2. Make tests more open-closed (as in the card vs. account case)
    3. Remove redundancy

    and all plays out well when I think of such tests as more acceptance or coarse-grained specifications, where creating an expressive API over a set of classes for testing is not only beneficial - it's necessary. However, as soon as I arrive at the at the unit level, the example leaves me a bit confused. Here's why:

    In your example, you refactor the test code to make it unaware of the 'card' abstraction. I seem to have few issues with the move itself and how it is done.

    1. I can see that you refactor to create the LoadFunds() (later LoadInitialFunds()) and LoadFunds() methods and claim that it removes redundancy. However, for a reader, this kind of redundancy might appear a little bit strange and if he read your book Essential Skills For The Agile Developer, he knows almost instantly that this is a special case of redundancy - one that does not violate the Shalloway's Principle (or, in other words, does not follow Shalloway's Law). As far as I remember, this is the kind of redundancy we both don't have to be afraid of and one that cannot be helped. To give you an example, let's imagine that one day, our customer tells us that he does not want to load funds anymore, but instead he'll have users load abstract units - "credits" where one credit's cost depends on whether a customer is a VIP customer or not. Given this requirement, the calls to LoadFunds() and MaximumFunds() would be redundant, because we'd have to change each of those to LoadCredits() and MaximumCredits() respectively in all places where they're called. This is not _much_ different from using an Account class instead of Card class.

    2. I'm having some hard times understanding how throwing exception is an implementation detail. As you wrote on your blog, 'client' is relative. So on unit level, a client of a class is another class. As I understand, unit level specs should be created from the point of view of their consuming entities, i.e. from Specification Perspective (i.e. how objects communicate). What think I see you trying to do is rise to the Conceptual Perspective, which in my opinion is too abstract for unit level. In other words, throwing exception does not seem like an implementation detail to me on unit level because if this is changed to anything other, then classes using the Account class will stop working. That alone seems like a fine reason to specify it explicitly.

    Of course, I'm probably missing something, but I think it's still valuable for you to know the points of concern or confusion for your readers :-).

    ReplyDelete
    Replies
    1. a. As for the type of redundancy, this one CAN be helped, we just showed how to get rid of it. Redundancies are NEVER good, and we may decided we can live with them, but if we don't have to, we don't :-)

      b. When you think of the system's behavior, then the customer wants to make funds available. You may rightfully argue that the abstraction that the customer is using (in their mind) is the CARD abstraction. In this case, a better name for the method could be LoadFundsIntoCard rather than just LoadFunds. Regardless of that, the actual name that we use for the class (CARD initially) can be encapsulated in the method.

      At this point, several things can change, for example, we can change FUNDS to CREDITS. Since we are TEST DRIVEN we will first change the methods' name to LoadCredits, etc. Once we have adjusted the tests to reflect requirement we may change the implementation to reflect the changed vocabulary:
      card.LoadCredits()
      which will force us to change our code.

      Summary:
      Requirement changed (name) ->
      Public test changed (use the name in test) ->
      Private method changed (force name change in code) ->
      Code changed


      The other change that we made was to go from card to account.
      The first question there is - what will the user see?
      Will the user still think of it as a CARD even though there is none present? Will the user say
      "charge it to my card" or "charge it to my account".

      The method name: LoadFunds() is independent of this discussion so it does not have to change.

      If we used LoadCreditsIntoCard and the customer's view is the former, we can leave the test untouched and just change the implementation inside the method.
      If it is the latter we need to rename this method to
      LoadCreditsIntoAccount

      Perhaps we just need to call the method
      LoadCustomerCredits (or LoadCustomerFunds) and do away with the CARD vs ACCOUNT distinction.

      The important point here is that the changes are done at two levels - the requirement level first and the design level second.


      The point is, whatever we do is driven from the tests.

      Delete
    2. Lets talk about the exception.

      The real thing that the client of the class needs is to know that something went wrong. There are several ways of doing so. Our favorite is an exception.

      Exceptions, however, are considered by some to be non-linear programming constructs because when you read some code you may or may not know that an exception may be thrown in it. This is why Java's standard exceptions are such a pain. You MUST deal with them in some form.

      So, maybe, we will just have the function return a return value object that we need to look at to determine what happened.

      If we create a function named (e.g.) ErrorIsReported() then we have taked two concerns and split them in two:
      The first concern is - what happens
      The second concern is - how does that happen

      The WHAT happens is in the public test, the implementation details are in the specific private method.

      NOW, and I want to make this perfectly clear. If your customer is talking in terms of exceptions, and they want nothing BUT exceptions, you can call that function ExceptionIsThrown(), you do not want to use a generic term (error) if your customer insists on a specific term. You can always change it later, right? But you still want to encapsulate the SPECIFIC exception, and where it is thrown from, so this can change, without affecting the public test.
      When you go from CARD to ACCOUNT, you will change the exception being thrown, righ?

      Delete
    3. Ok, I think I get it.

      My confusion was caused by me thinking that you're treating a reference to the card _domain_term_ as redundancy and trying to get rid of it by dropping the "card" word and leaving just loading funds. If this was the case, then each time something would change, we'd protect from the change by dropping another domain term, making the LoadFunds() method as DoSomething() :-D.

      Now I can see that what you're proposing is not dropping domain vocabulary, but rather encapsulating the implementation behind another layer (something we could call an "intention layer") that can be shaped outside the constraints pushed on the production classes (e.g. production classes require impose certain usage patterns based on how they're used by another production code).

      Is my understanding correct?

      Also, I think I missed one thing about the exception encapsulation. While I believed that the encapsulation is not that important because sometimes I still want to tell that this, not that exception kind is thrown, I did not draw enough conclusion from the appearance of the Funds() method you added to the exception. Now let's suppose there were more such methods on the exception (e.g. a card number and a user name), then we'd bloat the specification with details such as how many methods there are that yield this data and how they should be called and what kind of data they return. In such case, the benefit of encapsulating it away is more clearly visible.

      One more question. As there are also objects in the system that do not implement any domain rules (an example would be a synchronizing proxy), would you test-driven them using the same technique, introducing wrapping methods like AssertObjectIsLockedForConcurrentAccess()? Or is this kind of separation intended only for places where domain rules are held?

      And one finishing thought - it would be REALLY powerful if you could show an "intention layer" that would allow safe transition of funds from being uint to a value type (like Money, with amount and currency). I thought about it for a minute but could not come up with a compelling solution. Also, this is a change that's quite likely to happen when a system grows, e.g. in some European Countries, you can pay with both local currency and Euros.

      Delete
  2. As for abstracting out the exception, I'd probably go with something like this:

    UserShouldBeNotifiedOfErrorUpon(() => LoadInitialFunds(insufficientFunds), with: insufficientFunds);

    or something similar.

    ReplyDelete
    Replies
    1. What would the code look like?

      Delete
    2. like this:

      void UserShouldBeNotifiedOfErrorUpon(Action action, uint with)
      {
       var expectedFunds = with;
       try
       {
        action();
        Assert.Fail("Card should have thrown a " + 
            typeof(Account.InsufficientFundsException).Name);
       }
       catch (Account.InsufficientFundsException exception)
       {
        Assert.AreEqual(expectedFunds, exception.Funds());
       }
      }

      Of course, it does not eliminate the exception type being used twice. Also, it might be made more general- purpose. Anyway, it does reach the goal of taking the exception detail away from the test body.

      Delete
  3. This comment has been removed by the author.

    ReplyDelete
  4. Then you might as well break those private methods out into an interface and hide the assertions behind an interface as well so that you can run the test in two contexts: one to prove it the product works and one to prove the test can fail.

    ReplyDelete