HTTPS SSH

Untestable - How Focusing on the Test-ability of a System Forces Thoughtful Design

This application doesn't do much. It's a simple command line application for interacting with some of the bitbucket REST services.

It's built like this:

$>mvn clean install

And it's run like this:

$>java -jar target/untestable-1.0-SNAPSHOT-jar-with-dependencies.jar
No command provided. Nothing to do.
Supported commands are:
'g-user'
'g-user-repos'

And in order to run these commands, you'll need a bitbucket account.

What is far more interesting, however, is the way this application evolved from its initial implementation to where it is now. The intention of this project is to demonstrate how focusing on the test-ability of an application forces thoughtful design. By looking through the commit history of this project, it should become clear how focusing on the problem of creating good tests is to focus on the problem on creating sound, flexible and robust software design.

This project endeavours not just to espouse the benefits of some of some of the approaches that you might have heard preached, but to understand the root of their motivation. This document will clearly illustrate and explain the evolution of the code base from the most naïve (but still technically correct) implementation, to a more sophisticated one.

Requirements

As with any approach for designing a system, it's best to start with a set of requirements that define exactly what the system should do. This system uses a set of requirements that are structured to be simple, so that we can quickly build an application, but interesting enough that we can recognize in this system some interesting patterns that would represent things that are actually encountered in the real world.

This system will allow the user to interact with the bitbucket REST API via the command line. It will solicit credentials and optionally remember them to be used during subsequent calls. The aim is to make it easy to get information out of bitbucket without needing to understand the intricacies of an interaction with a REST based service over HTTP. Here are the enumerated, detailed requirements that should provide all the info needed to understand what this system needs to do.

  1. When the user provides a named command as argument to the application identifying the bitbucket service to call, the application will make that REST call. The 'g-user' command will identify the GET a user profile service. The 'g-user-repos' command will identify the GET a list of repositories visible to an account service
  2. When the 'g-user' command is run, the application will call the GET a user profile service and modify the response body by removing the "repositories" element and then display the modified content back to user; nicely formatted and easily readable.
  3. When the 'g-user-repos' command is run, the application will call the GET a list of repositories visible to an account service and then display the body of that response back to th user; nicely formatted and easily readable.
  4. When the user does not provide any command as argument, the application will display a message listing the available commands
  5. When the user provides a command to the application, the application will solicit a username and password from the user that will be used to authenticate access to the service. The application will also ask if the user would like to have the credentials remembered for future use.
  6. If the user elects to remember the credentials, the application will save them somewhere that can be accessed by the application the next time the application is run.
  7. If the user elects not to remember the credentials, the application will not save them, and on subsequent runs, the user will need to re-enter the username and password.
  8. When the user runs the application with a command and there are a previously remembered set of credentials available to the application, the application will use the previously remembered set of credentials (rather than prompting the user for them) to authenticate access to the service
  9. If any command is run, and the call to the bitbucket REST service returns an error, a similar error message will be displayed back to the user.

As you can see, the requirements are pretty straightforward, but still give quite a bit of space within which to build a system that highlights some of the most critical aspects of building clear, concise and testable software.

tag: 1-initial-state

This is the most naïve implementation. It purposefully commits every cardinal sin of software design:

  1. everything is static
  2. it accesses its resources (the console and the file system) directly
  3. the entire application is coded up in the 'static void main(String[])' method.
  4. etc.

At least it has one thing going for it... the component diagram is pretty easy to draw:

             +------------+                  
             | Untestable |                  
        +----+            +--------+         
        |    |            |        |         
        |    +-----+------+        |         
   Uses |     Uses |          Uses |         
        |          |               |         
+-------v-+  +-----v-------+   +---v-------+
| Console |  | File System |   | Bitbucket |
|         |  |             |   |           |
+---------+  +-------------+   +-----------+

In this implementation, it is practically impossible write a unit test. There are two problems that need to be addressed before it will be possible:

  1. There is no way to provide the application with user input and no way to determine what the application displays to the user. This makes it impossible to test that the application behaves the way it is supposed to under various scenarios.
  2. The application accesses the console via java.lang.System.console(), which only has a value when java is launched through the console. Since the test is run with a JUnit runner, the JVM has no console defined and the first attempt to write to it throws an NPE... wonderful!

The best that can be done is to enumerate the set of tests that break down the requirements and serve as a placeholder for where we will come back around to write the test code once it is actually possible to do so. For example, here is a test that is focused on validating the correct behavior of the system against requirement #2. From UntestableTest.java:

/**
 * Test that when 'g-user' is called with valid credentials, the bitbucket user profile service is called and
 * the results are displayed back to the user
 */
@Ignore
public void test4() throws Exception {

    //PRE CONDITIONS:
    //When there are no stored credentials
    //And when the username is valid
    //And when the password is valid
    //And when the user doesn't want to have the credentials remembered
    //And when the bitbucket getUser service responds with user profile information

    //OPERATION:
    //And the app is run with the "g-user" command.
    Untestable.main(new String[]{"g-user"});

    //POST CONDITIONS:
    //The app should collect the username
    //The app should collect the password as a piece of sensitive information
    //The app should ask if the user wants their credentials remembered
    //The app should not persist the credentials
    //The app should display the user profile information, formatted nicely, with the "repositories" property removed
}

Here, the requirement is broken into three sections, the "PRE CONDITIONS", the "OPERATION" and the "POST CONDITIONS". With no ability to control the pre conditions or validate the post conditions, it's clear that this requirement is not possible to test at the moment. This is exactly what we will look to remedy throughout the next few iterations of the design.

tag: 2-using-existing-interfaces-for-io

This changes the application by instead reading from a BufferedReader and writing to a PrintStream. Additionally, the application delays the creation of these things until they are actually referenced for the first time, using a psuedo-singleton pattern, like this:

/**
 * Get an initialized BufferedReader for gathering user input. If one has not been set, System.in is used
 * @return
 */
static BufferedReader in(){
    if(i ==null)
        i = new BufferedReader(new InputStreamReader(System.in));
    return i;
}
static BufferedReader i = null;

This gives the unit tests a chance to inject a mocked value before the main method is called.

This change is meant to illustrate the value of programming to an interface, because it begins to make this application at least possible to test. The setup of the unit test looks like this

 //And when the username is valid
 String input =
 "tcbakes\n" +
 //And when the password is invalid
 "fake-o\n" +
 //And when the user doesn't want to have the credentials remembered
 "n\n";

 ...

 /**
  * SETUP: Define the input to the test as newline separated string.
  * Build up a BufferedReader for the app to use
  */
  Untestable.i = new BufferedReader(new InputStreamReader(new ByteArrayInputStream(input.getBytes())));

The test takes advantage of the ability to control the behavior of the BufferedReader used by the application and is able to set up some test input that will be used to drive the test case.

The general pattern for creating a good unit test is to isolate the component under test (in this case Untestable.java) and mock out and control the behavior of its dependent components to acheive a desired path through a use case. It can be illustrated like this:

             +-----------+        
             | Test      |        
   +---------+           |        
   |         +----+------+        
   |              | Invokes       
   |              v               
   |        +-----+------+        
   |        | Component  |        
Creates/    | Under      +------+ 
Controls    | Test       |      | 
   |        +------------+      | 
   |                            | 
   |      +-------+           Uses
   |      |  +--------+         | 
   +----> |  Mocked------+      | 
          +--|  Dependencies    | 
             +--|        | <----+ 
                +--------+

There are still a number of other issues with the application (we haven't addressed the tight coupling to bitbucket or to the file system, yet) but at least it's headed in the right direction. The two issues that we'll highlight here and fix in the next commit are:

  1. The ability to collect information from the user has now become a little bit more muddled. Access to the java.io.Console allows us to use the readLine(String, Object...) method, which both displays and collects information in one expression. Without access to this method (which isn't defined in the interface that is being used), we're left to split this out into two lines, which is a bit less readable.
  2. The other issue is that we've actually broken something. The java.io.Console class that was used previously offers a method called readPassword() that turns off echoing for any characters typed. This is exactly the behavior needed. However, now that the application uses a PrintStream, which is designed for a higher level of abstraction and doesn't offer this kind of functionality, the problem of turning echoing on/off becomes a problem that we are left to solve. It seems silly to have to re-invent the wheel here, which leads to...

tag: 3-custom-interfaces-for-io

In the interest of re-using as much readily available functionality as possible, a brand new interface is defined, UserIO.java, that describes exactly the behavior needed. Here's the interface definition:

/**
 * Prompt a user for information. Display information back to a user.
 */
public interface UserIO {

    /**
     * Prompt a user for information.
     * @param prompt A format string
     * @param args optional arguments for the format string
     * @return a value entered by the user in response to the prompt
     */
    String collect(String prompt, Object... args);

    /**
     * Similar to {@link #collect(String, Object...)}, except that the i
     * nformation being collected is sensitive, so care should be taken to appropriately
     * obscure/protect the information during display
     *
     * @param prompt A format string describing the information needed
     * @param args optional arguments for the format string
     * @return a value entered by the user in response to the prompt
     */
    String collectSensitive(String prompt, Object... args);

    /**
     * Display information to the user
     *
     * @param info The information to display
     */
    void display(String info);
}

The consumer of this interface can acheieve exactly the behavior needed and do so in an clear and expressive way. The implementation of this interface will be nicely decoupled from the consumer and is free to make whatever implementation choices are most appropriate. ConsoleIO.java implements this interface, and uses java.io.Console to provide a super straightforward implementations of each one of these methods. For instance, here's the implementation of the (previously broken) password collection functionality:

@Override
public String collectSensitive(String prompt, Object... args) {
    return new String(console().readPassword(prompt, args));
}

With the introduction of precisely defined customer interfaces, we really can have our cake and eat it, too.

The modified sketch of the system's key components looks like this:

                         +------------+                 
                         | Untestable |                 
   +----------------+----+            +-------+         
   |                |    |            |       |         
   |                |    +-----+------+       |         
   |             Uses          |              |         
Creates             |          |              |         
   |        +-------v-+  +-----v-------+   +--v--------+
   |        | UserIO  |  | File System |   | Bitbucket |
   |        |         |  |             |   |           |
   |        +----^----+  +-------------+   +-----------+
   |             |                                      
   |  Implements |                                      
   |         +---+-----+                                
   +-------> |ConsoleIO|                                
             |         |                                
             +---+-----+                                
            Uses |                                      
                 |                                      
             +---v-----+                                
             |Console  |                                
             |         |                                
             +---------+

The unit tests are now much easier to write, too. The unit test defines a mock implementation of ConsoleIO called MockIO.java. MockIO.java does nothing more than replay a list of user inputs provided during creation of a MockIO instance, and remember a list of outputs for later inspection and validation. Here's one of the unit tests from UntestableTest.java:

/**
 * Test that when 'g-user' is given a bad password, an error is
 * appropriately written to the output.
 */
@Test
public void test2() throws IOException, UnirestException {

    //PRE CONDITIONS:
    //When there are no stored credentials
    //And when the username is valid
    MockIO mock = new MockIO(asList(
    "tcbakes",
    //And when the password is invalid
    "fake-o",
    //And when the user doesn't want to have the credentials remembered
    "n"));
    //And when the bitbucket getUser service responds with "Unauthorized"

    /**
     * SETUP: inject the MockIO instance
     */
    Untestable.io = mock;

    //OPERATION:
    //And the app is run with the "g-user" command.
    Untestable.main(new String[]{"g-user"});

    //POST CONDITIONS:
    assertListEquals(asList(
            //The app should collect the username
            "Username: ",
            //The app should collect the password as a piece of sensitive information
            "Password: ",
            //The app should ask if the user wants their credentials remembered
            "Remember [y/n]: ",
            //The app should display the "unauthorized" message back to the user
            "Problem calling the service. Response code: 401"), mock.getGeneratedOutputs());
            //The app should not persist the credentials

}

This is pure poetry compared to what this test used to look like.

At this point, it should start becoming clear how the problem of writing concise and elegant unit tests really boils down to designing concise and elegant software. And the inverse is equally true; poor unit tests are often a symptom of poor design.

There is one aspect of this implementation that still isn't quite right: Untestable.java continues to reference the implementation class, even with the change to programming against the UserIO.java interface. Untestable.java is still burdened with the responsibility of initializing an instance of ConsoleIO and in this way, it continues to be inappropriately coupled to an implementaiton for with which it has no business managing. The piece of code that is coming under scrutiny is right here:

/**
 * Gets a {@link UserIO} instance for collecting info from and displaying info
 * to the user.  If the {@link UserIO} instance has not been set, then a default
 * {@link ConsoleIO} instance will be created and returned.
 * @return
 */
static UserIO io(){
    if(io == null)
        io = new ConsoleIO();
    return io;
}

This leads to the next fix...

tag: 4-DI-and-IoC-with-spring

The Inversion of Control pattern allows us to more-or-less flip the direction of the dependency. With this pattern Untestable will rely only on the interfaces, ConsoleIO will only worry about implementing the interfaces, and the control over the wiring of an initialized ConsoleIO instance into an Untestable instance will be given to a Dependency Injection framework whose injection behavior is driven off the declarative configuration, ConsoleConfig. Here's the new sketch:

                                         +----------------------------------+      
                                         |                                  |      
                              +----------v---------+                        |      
    +-----------------------> | BitbucketClientApp |                        |      
    |                    +----+                    +----+                   |      
Creates and              |    |                    |    |                   |      
Injects Dependency       |    +--------------------+    |                   |      
    |                 Uses       Uses                Uses                   |      
    |                    |          |                   |                   |      
    |            +-------v-+  +-----v-------+   +-------v---+             Uses     
    |            | UserIO  |  | File System |   | Bitbucket |               |      
    |            |         |  |             |   |           |               |      
    |            +----^----+  +-------------+   +-----------+               |      
    |                 |                                                     |      
    |      Implements |                                                     |      
    |             +---+-----+                                        +------+-----+
    |   +-------> |ConsoleIO|                                        | Untestable |
    |   |         |         |                                        |            |
    |   |         +---+-----+                                        +------+-----+
    |   |        Uses |                                                     |      
    |   |             |                                                     |      
    | Creates     +---v-----+                                               |      
    |   |         |Console  |                                             Uses     
    |   |         |         |                                               |      
    |   |         +---------+                                               |       
    |   |                       +--------------+                            |      
    +---+-----------------------+ DI Framework <----------------------------+      
                                |              |                                   
                                +--------^-----+                                   
                                         | Drives                                  
                                         |                                         
                                 +-------+-------+                                 
                                 | ConsoleConfig |                                 
                                 |               |                                 
                                 +---------------+

The Dependency Injection Framework of choice in this implementaiton is Spring, but there are a number of frameworks out there that do the same thing in the interest of tackling the same problem.

In this implementation, the role of Untestable.java has been redefined and new component is introduced.

  • Untestable.java - Is relegated to the role of a simple boot-strapper. It spins up the DI Framework, points it to the ConsoleConfig and then asks for a fully constructed and initialized BitbucketClientApp instance. Actual execution of the app is delegated to the BitbucketClientApp instance
  • BitbucketClientApp.java - A new component that takes over the role that Untestable.java used to own.

With the dependency injection framework now in place, this chunk of code:

/**
 * Gets a {@link UserIO} instance for collecting info from and displaying info
 * to the user.  If the {@link UserIO} instance has not been set, then a default
 * {@link ConsoleIO} instance will be created and returned.
 * @return
 */
static UserIO io(){
    if(io == null)
        io = new ConsoleIO();
    return io;
}

Can be replaced with this one:

/**
 * A {@link UserIO} instance for collecting info from and displaying info
 * to the user.
 * @param io
 */
private UserIO io;

@Autowired
public void setIo(UserIO io) {
    this.io = io;
}

(Note: 'Autowired' is an instruction to the Spring framework to inject this dependency with a configured instance of a UserIO)

Although all this work currently only manifests itself in this subtle removal of a call to "new Console()", it represents a very important improvement, flipping the entire problem of the direction of the dependencies on its head so that, moving forward, the system is in a position to evolve right along with the requirements without having to totally overhaul the whole thing.

tag: 5-catching-up

It's now time to catch the other components of the system up to the design paradigm introduced in previous commits... The concepts of 'programming to an interface' and 'dependency injection' are used in the introduction of the following:

The system sketch now looks like this:

                                         +----------------------------------+      
                                         |                                  |      
                                         |                                  |      
                                         |                                  |      
                              +----------v---------+                        |      
    +-----------------------> | BitbucketClientApp |                        |      
    |                    +----+                    +----------+             |      
    |                    |    |                    |          |             |      
Injects Dependency       |    +--------------------+          |             |      
    |                 Uses             Uses                 Uses            |      
    |                    |               |                    |             |      
    |            +-------v-+       +-----v-------+      +-----v-----+     Uses     
    |            | UserIO  |       | Credential  |      | Bitbucket |       |      
    |            |         |       | Persistence |      | Client    |       |      
    |            +-^-------+       +-^-----------+      +-^---------+       |      
    |        Implements         Implements          Implements              |      
    |              |                 |                    |                 |      
    |      +-------+---+       +-----+-------+     +------+-------+  +------+-----+
    |  +-> | ConsoleIO |  +--> | FileBased   |     | RestBBClient |  | Untestable |
    |  |   |           |  |    | CredPersist |  +> |              |  |            |
    |  |   +-----+-----+  |    +-----+-------+  |  +------+-------+  +------+-----+
    |  |         |        |          |          |         |                 |      
    | Creates  Uses    Creates     Uses     Creates     Uses                |      
    |  |         |        |          |        |           |                 |      
    |  |   +-----v----+   |    +-----v------+ |     +-----v-----+         Uses     
    |  |   | Console  |   |    | FileSystem | |     |Bitbucket  |           |      
    |  |   +----------+   |    +------------+ |     +-----------+           |      
    |  |                  |    +--------------+                             |      
    +--+------------------+----+DI Framework  <-----------------------------+      
                               |              |                                     
                               +-------^------+                                     
                                       | Drives                                    
                                       |                                           
                               +-------+-------+                                   
                               | ConsoleConfig |                                   
                               |               |                                   
                               +---------------+

The most important aspect of this change is that we are now finally able to cover the full set of functionality in our unit tests. Here's a unit test included in this implementation that was previously impossible to write:

BitbucketClientAppTest.java#test4

/**
 * Test that when 'g-user' is called with valid credentials, the bitbucket user profile service is called and
 * the results are displayed back to the user
 */
@Ignore
public void test4() throws Exception {

    BitbucketClientApp app = new BitbucketClientApp();

    //PRE CONDITIONS:
    //When there are no stored credentials
    app.setCp(new CredentialPersistence() {
        @Override
        public void persist(Credentials creds) throws Exception {
            fail("Credentials should not be persisted if the user selects 'n'");
        }

        @Override
        public Credentials retrieve() throws Exception {
            return null;
        }
    });

    MockIO mock = new MockIO(asList(
            //And when the username is valid
            "tcbakes",
            //And when the password is valid
            "valid123",
            //And when the user doesn't want to have the credentials remembered
            "n"));
    app.setIo(mock);

    //And when the bitbucket getUser service responds with user profile information
    app.setBb(new BitbucketClient() {
        @Override
        public BitbucketResponse getUser(Credentials creds) {
            return new BitbucketResponse(HttpStatus.SC_OK, "OK", new JsonNode(
                    "{\n" +
                            "    \"repositories\": [],\n" +
                            "    \"user\": {\n" +
                            "        \"username\": \"tcbakes\",\n" +
                            "        \"first_name\": \"Tristan\",\n" +
                            "        \"last_name\": \"Baker\",\n" +
                            "        \"display_name\": \"Tristan Baker\",\n" +
                            "        \"is_staff\": false,\n" +
                            "        \"avatar\": \"https://bitbucket.org/account/tcbakes/avatar/32/?ts=0\",\n" +
                            "        \"resource_uri\": \"/1.0/users/tcbakes\",\n" +
                            "        \"is_team\": false\n" +
                            "    }\n" +
                            "}"));
        }

        @Override
        public BitbucketResponse getUserRepos(Credentials creds) {
            return null;
        }
    });

    //OPERATION:
    //And the app is run with the "g-user" command.
    app.run(new String[]{"g-user"});

    //POST CONDITIONS:
    assertListEquals(asList(

            //The app should collect the username
            "Username: ",
            //The app should collect the password as a piece of sensitive information
            "Password: ",
            //The app should ask if the user wants their credentials remembered
            "Remember [y/n]: ",
            //The app should display the user profile information, formatted nicely, with the "repositories" property removed
            "{\"user\": {\n" +
                    "    \"is_staff\": false,\n" +
                    "    \"resource_uri\": \"/1.0/users/tcbakes\",\n" +
                    "    \"last_name\": \"Baker\",\n" +
                    "    \"avatar\": \"https://bitbucket.org/account/tcbakes/avatar/32/?ts=0\",\n" +
                    "    \"display_name\": \"Tristan Baker\",\n" +
                    "    \"first_name\": \"Tristan\",\n" +
                    "    \"is_team\": false,\n" +
                    "    \"username\": \"tcbakes\"\n" +
                    "}}"), mock.getGeneratedOutputs());
    //The app should not persist the credentials

}

But man, do these tests look wordy! Ideally, we want the tests to be as clear in their intention as possible, and while the use of anonymous inner classes to create on-the-fly mock behavior certainly gets the job done, it doesn't do much for readability.

In the next section, we'll tackle exactly this problem.

tag: 6-fluent-tests-with-mockito

In this section, the focus is purely on writting better unit tests. The sketch of the system remains exactly the same. It is here where we finally get to reap the rewards of all of our hard work!

Mockito (and other libraries like it) provide two really key pieces of functionality

  1. The ability to dynamically create mock classes and control their bejavior. (this is almost always done under the covers using the CGLIB library, which enables runtime bytecode manipulation - very clever stuff!)
  2. A carefully designed fluent interface aimed at getting as close as possible to test code that reads in English like the requirements they are testing

Here's what the unit tests look like now:

BitbucketClientAppTest.java#test3

/**
 * Test that when 'g-user' is called with valid credentials, 
 * the bitbucket user profile service is called and
 * the results are displayed back to the user
 */
@Test
public void test3() throws Exception {

    //When there are no stored credentials
    when(mockCp.retrieve()).thenReturn(null);
    //And when the username is valid
    when(mockIo.collect("Username: ")).thenReturn("tcbakes");
    //And when the password is valid
    when(mockIo.collectSensitive("Password: ")).thenReturn("fake-o");
    //And when the user doesn't want to have the credentials remembered
    when(mockIo.collect("Remember [y/n]: ")).thenReturn("n");
    //And when the bitbucket getUser service responds with user profile information,
    // not nicely formatted
    // (note that in this payload, indentation has been removed)
    when(mockBb.getUser(any(Credentials.class)))
            .thenReturn(new BitbucketResponse(HttpStatus.SC_OK, "OK", new JsonNode(
                    "{\n" +
                     "\"repositories\": [],\n" +
                     "\"user\": {\n" +
                     "\"username\": \"tcbakes\",\n" +
                     "\"first_name\": \"Tristan\",\n" +
                     "\"last_name\": \"Baker\",\n" +
                     "\"display_name\": \"Tristan Baker\",\n" +
                     "\"is_staff\": false,\n" +
                     "\"avatar\": \"https://bitbucket.org/account/tcbakes/avatar/32/?ts=0\",\n" +
                     "\"resource_uri\": \"/1.0/users/tcbakes\",\n" +
                     "\"is_team\": false\n" +
                     "}\n" +
                     "}")));

    //And the app is run with the "g-user" command.
    app.run(new String[]{"g-user"});

    //The app should collect the username
    verify(mockIo).collect(eq("Username: "));
    //The app should collect the password as a piece of sensitive information
    verify(mockIo).collectSensitive(eq("Password: "));
    //The app should ask if the user wants their credentials remembered
    verify(mockIo).collect(eq("Remember [y/n]: "));
    //The app should not persist the credentials
    verify(mockCp, never()).persist(any(Credentials.class));
    //The app should display the user profile information, formatted nicely,
    // with the "repositories" property removed
    verify(mockIo).display(eq("{\"user\": {\n" +
                                "    \"is_staff\": false,\n" +
                                "    \"resource_uri\": \"/1.0/users/tcbakes\",\n" +
                                "    \"last_name\": \"Baker\",\n" +
                                "    \"avatar\": \"https://bitbucket.org/account/tcbakes/avatar/32/?ts=0\",\n" +
                                "    \"display_name\": \"Tristan Baker\",\n" +
                                "    \"first_name\": \"Tristan\",\n" +
                                "    \"is_team\": false,\n" +
                                "    \"username\": \"tcbakes\"\n" +
                                "}}"));
}

Now this is starting to look pretty cool!

We've come a long way since 1-initial-state and ended up with a suite of unit tests that very clearly and concisely describe the requirements they are testing at the same time that they test them.

tag: 7-defining-smaller-units

If we spend some time looking through the implementation of BitbucketClientApp and think about what it would mean to introduce new commands to the system, we can pretty quickly see how the BitbucketClientApp's responsibilities would grow right along with it, and the code will get longer and longer and certainly eventually present a maintenance headache.

Additionally, as we review the set of test cases and the requirements that they describe, it should be clear that certain rules can be extracted out of those tests that are generally applicable across the entire set of commands. The monolithic nature of the BitbucketClientApp is reflected in the monolithic nature of each of the requirements described in the unit tests.

Think for a moment about these two requirements as they are currently described in BitbucketClientAppTest.java

Here's a requirement from test1():

    When there are no stored credentials
    And when the username is valid
    And when the password is invalid
    And when the user doesn't want to have the credentials remembered
    And when the bitbucket getUser service responds with "Unauthorized"

    And the app is run with the "g-user" command.

    The app should collect the username
    The app should collect the password as a piece of sensitive information
    The app should ask if the user wants their credentials remembered
    The app should not persist the credentials
    The app should display the "unauthorized" message back to the user

Here's a requirement from test2():

    When there are no stored credentials
    And when the username is valid
    And when the password is invalid
    And when the user doesn't want to have the credentials remembered
    And when the bitbucket getUserRepos service responds with "Unauthorized"

    And the app is run with the "g-user-repos" command.

    The app should collect the username
    The app should collected the password as a piece of sensitive information
    The app should ask if the user wants their credentials remembered
    The app should not persist the credentials
    The app should display the "unauthorized" message back to the user

And here's one from test7():

    When there are no stored credentials
    And when the username is valid
    And when the password is valid
    And when the user doesn't want to have the credentials remembered

    And an unrecognized command is passed

    The app should collect the username
    The app should collect the password as a piece of sensitive information
    The app should ask if the user wants their credentials remembered
    The app should display a message back to the user explaining that the provided command is not supported

While all these statements are certainly valid according to the requirements, they are very redundant. In order to be more concise, we could instead define rules that are generally applicable across the entire system, regardless of the specifics of any given requiremnent. There are at least 2 rules here

  1. A user's request for a command should be routed to a specific command capable of handling that request; if no command exists to support the user's request, an error message should be displayed.
  2. Commands requiring credentials should solicit them from the user

Rule #1 will be the focus of this section, and rule #2 will be addressed in a subsequent section

Rule #1 justifies the existence of a "routing" component whose sole purpose is to interpret a user's request and route it to the right place for execution. Since this requirement will be true regardless of the introduction of new commands over the course of the life of the system, we should be able to design a component whose behavior will not change just because a new command is introduced. Making a little bit of investment up from to better ensure the longevity of a component as the system evolves is usually worth the effort.

In order to implement this rule, we change the role of the BitbucketClientApp from that of a one-stop-shop to that of a simple request router. We'll use the visitor pattern in order to decouple the router from the implementation of a command. We introduce Command.java as an interface that generically describes the capabilities of a command.

Here is Command.java:

/**
 * Something that interacts with a user to perform some useful activity
 * and is identifiable by a simple string command name.
 */
public interface Command {

    /**
     * Returns true if this can candle the command
     * @param command The name of the command
     * @return true if this can handle the request, false otherwise
     */
    boolean canHandle(String command);

    /**
     * The name of the command
     * @return the name
     */
    String getName();

    /**
     * Performs the command
     * @throws Exception an unrecoverable error occurs
     */
    void run() throws Exception;

}

And here is the modified sketch of the system in light of this new design:

                                +--------------------+                                  
                                | BitbucketClientApp |                                  
         +--------------------> |                    | <-------------------------+      
         |                      +---------+----------+                           |      
         |                                |                                      |      
     Injects                    Aggregates and Routes To                         |      
     Dependency                           |                                      |      
         |                         +------v------+                               |      
         |                         |  Command    |                               |      
         |                 +------->             <----------+                  Uses     
         |           Implements    +-------------+      Implements               |      
         |                 |                                |                    |      
    Injects-----------> +--+--------+           +-----------+-+                  |      
    Dependency          | GUser     |           | GUserRepos  |         +--------+-----+
         |              | Command   +------+----+ Command     |         |  Untestable  |
         |  +---------> +-----------+      |    +-------------+         +--------+-----+
         |  |                            Uses                                    |      
         |  Creates                        |                                     |      
         |  |              +--+------------+---+--+-------------+--+             |      
         |  |              |  |                |  |             |  |           Uses     
         |  |         +----v--v-++      +------v--v---+      +--v--v-----+       |      
         |  |         | UserIO  |       | Credential  |      | Bitbucket |       |      
         |  |         |         |       | Persistence |      | Client    |       |      
         |  |         +---------+       +-------------+      +-----------+       |      
         |  |     Implements         Implements          Implements              |      
         |  |           |                 |                    |                 |      
         |  |   +-------+---+       +-----+-------+     +------+-------+         |      
         |  +-> | ConsoleIO |  +--> | FileBased   |     | RestBBClient |         |      
         |  |   |           |  |    | CredPersist |  +> |              |         |      
         |  |   +-----+-----+  |    +-----+-------+  |  +------+-------+         |      
         |  |         |        |          |          |         |                 |      
         | Creates  Uses    Creates     Uses     Creates     Uses                |      
         |  |         |        |          |        |           |                 |      
         |  |   +-----v----+   |    +-----v------+ |     +-----v-----+           |      
         |  |   | Console  |   |    | FileSystem | |     |Bitbucket  |           |      
         |  |   |          |   |    |            | |     |           |           |      
         |  |   +----------+   |    +------------+ |     +-----------+           |      
         |  |                  |      +------------++                            |      
         +--+------------------+------+DI Framework <----------------------------+      
                                      |             |                                   
                                      +-------^-----+                                   
                                              | Drives                                  
                                              |                                         
                                      +-------+-------+                                 
                                      | ConsoleConfig |                                 
                                      |               |                                 
                                      +---------------+

With this new design, we can focus the details of the requirements of what a command should do within a more focused set of tests that deal only with a particular command.

For example, here is a test from GUserCommandTest.java:

/**
 * Test that when 'g-user' is given a bad password, an error is
 * appropriately written to the output.
 */
@Test
public void test1() throws Exception {

    //When there are no stored credentials
    when(mockCp.retrieve()).thenReturn(null);

    //And when the username is valid
    when(mockIo.collect("Username: ")).thenReturn("tcbakes");
    //And when the password is invalid
    when(mockIo.collectSensitive("Password: ")).thenReturn("fake-o");
    //And when the user doesn't want to have the credentials remembered
    when(mockIo.collect("Remember [y/n]: ")).thenReturn("n");
    //And when the bitbucket getUser service responds with "Unauthorized"
    when(mockBb.getUser(any(Credentials.class)))
            .thenReturn(new BitbucketResponse(HttpStatus.SC_UNAUTHORIZED, "Unauthorized", null));

    //And the command is run
    command.run();

    //The command should collect the username
    verify(mockIo).collect(eq("Username: "));
    //The command should collect the password as a piece of sensitive information
    verify(mockIo).collectSensitive(eq("Password: "));
    //The command should ask if the user wants their credentials remembered
    verify(mockIo).collect(eq("Remember [y/n]: "));
    //The command should not persist the credentials
    verify(mockCp, never()).persist(any(Credentials.class));
    //The command should display the "unauthorized" message back to the user
    verify(mockIo).display(eq("Problem calling the service. Response code: 401"));

}

Importantly, note that the sentence

//And the "g-user" command is run

Has now changed to

//And the command is run

Implying that any logic around command routing has already been handled by the routing rule that is the focus of our efforts in this implementation.

Also, the tests in BitbucketClientAppTest.java are a very clear reflection of the exact behaviors of this routing rule:

test1():

/**
 * A recognized command should be delegated to.
 */
@Test
public void test1() throws Exception {

    //When a command exists capable of handling the request
    when(mockCommand.canHandle(anyString())).thenReturn(true);

    //And the user makes that request
    app.run(new String[]{"any-command"});

    //The app should delegate the running of the command to the command instance
    verify(mockCommand).run();
}

and test2():

/**
 * An unrecognized command should display an error to the user.
 */
@Test
public void test2() throws Exception {

    //When there does not exist a command of handling the request
    when(mockCommand.canHandle(anyString())).thenReturn(false);

    //And the user makes that request
    app.run(new String[]{"any-command"});

    //The app should not call the command
    verify(mockCommand, never()).run();
    //The app should display a message back to the user explaining that the provided command is not supported
    verify(mockIo).display(eq("Unrecognized command 'any-command'"));
}

Now, on to tackle Rule #2...

tag: 8-defining-common-units

To recap: we've noted the repeated requirement of credential collection and management. Given that it's sprayed accross almost all of our unit tests, it's pretty hard to miss. From this, we have concluded the existence of the following over-arching rule:

  • Commands requiring credentials should solicit them from the user

The details of exactly how to collect and manage credentials are also detailed throughout the tests. The goal will be to define an approach for re-using an implementation of this rule across all commands that might need credentials.

This is done by defining a re-usable component that we will term a "flow".

Here is a look at Flow.java

/**
* A re-usable user interaction that displays/collects information
    * @param <O> The type encapsulating the data collected by the flow
*/
public interface Flow<O> {

    /**
    * Runs the flow, returns an object encapsulating the data collected
        * @return The data collected
        * @throws Exception some unrecoverable error has occured
    */
    O run() throws Exception;
}

And here is a portion of the implementation from SimpleCredentialCollectionFlow.java:

/**
* A re-usable flow for collecting credentials
*/
public class SimpleCredentialCollectionFlow implements Flow<Credentials> {
...

    @Override
    public Credentials run() throws Exception {
        //See if there are any stored credentials to use
        Credentials creds = cp.retrieve();

        //Collect them
        if (creds == null) {
            creds = new Credentials(
                    io.collect("Username: "),
                    io.collectSensitive("Password: "));

            //See if the user would like them persisted for use next time
            if (io.collect("Remember [y/n]: ").equalsIgnoreCase("y")) {
                cp.persist(creds);
            }
        }

        return creds;
}

Note that the body of this method looks exaclty like the first portion of the implementation of the GUserCommand.java and GUserReposCommand.java run methods used to look like. This is because we have defined an approach that allows us to factor out and encapsulate the implementation of the management of credentials into a common component that other commands can use.

A command can now declare a dependency on a Flow that is capable of collecting credentials like this:

/**
 * A user flow that will collect credentials
 */
private Flow<Credentials> cf;

@Autowired
public void setCf(Flow<Credentials> cf){
    this.cf = cf;
}

And initiate the collection of the credentials like this (from GUserCommand.java):

@Override
public void run() throws Exception{

    Credentials creds = cf.run();

    BitbucketResponse response = bb.getUser(creds);

    if(response.statusCode == 200) {
        response.responseBody.getObject().remove("repositories");
        org.json.JSONObject body = response.responseBody.getObject();
        io.display(body.toString(4));
    } else {
        io.display("Problem calling the service. Response code: " + response.statusCode);
    }
}

And importantly, notice how much more concise the unit tests are. Everything about testing the details of credential collection are focused in SimpleCredentialCollectionFlowTest.java, with no mention or need to handle command routing or Bitbucket rest service calls:

/**
 * When there are no stored credentials, the flow should collect them from the user
 */
@Test
public void test1() throws Exception {

    //When there are no stored credentials
    when(mockCp.retrieve()).thenReturn(null);
    //And when a username is provided
    when(mockIo.collect("Username: ")).thenReturn("tcbakes");
    //And when a password is provided
    when(mockIo.collectSensitive("Password: ")).thenReturn("fake-o");
    //And when the user doesn't want to have the credentials remembered
    when(mockIo.collect("Remember [y/n]: ")).thenReturn("n");

    //And the flow is run
    Credentials creds = flow.run();

    //The command should collect the username
    verify(mockIo).collect(eq("Username: "));
    //The command should collect the password as a piece of sensitive information
    verify(mockIo).collectSensitive(eq("Password: "));
    //The command should ask if the user wants their credentials remembered
    verify(mockIo).collect(eq("Remember [y/n]: "));
    //The command should not persist the credentials
    verify(mockCp, never()).persist(any(Credentials.class));

    //And the flow should return the collected credentials
    assertEquals("tcbakes", creds.username);
    assertEquals("fake-o", creds.password);

}

And in the unit tests for the commands, there is no need or mention of anything other than the simple presence of credentials.

From GUserCommandTest.java:

/**
 * The command should appropriately hand errors from the service and display
 * a message
 */
@Test
public void test1() throws Exception {

    //When there are credentials
    when(mockFl.run()).thenReturn(new Credentials("foo", "bar"));
    //And when the bitbucket getUser service responds with and error
    when(mockBb.getUser(any(Credentials.class)))
            .thenReturn(new BitbucketResponse(HttpStatus.SC_UNAUTHORIZED, "Unauthorized", null));

    //And the command is run
    command.run();

    //The command should display a simple explanation of the cause of the error
    verify(mockIo).display(eq("Problem calling the service. Response code: 401"));

}

Finally, here is our new system design:

                          +--------------------+                                    +----------------+
                          | BitbucketClientApp |                                    |  ArgDrivenApp  |
   +--------------------> |                    | +---------Implements--------------->                |
   |                      +---------+----------+                                    +---------+------+
   +                                +                                                         ^
 Injects                  Aggregates and Routes To                                            |
 Dependency                         +                                                         |
   +                          +-----+---+                                                     |
   |                          | Command |                             +------+                |
   |                 +-------->         <-------------+   +-----------> Flow |                |
   |           Implements     +---------+      Implements |  +-------->      |                |
   +                 +                                +   |  |        +--^---+              Uses
Injects+--------> +--+--------+           +-----------++  |  |       Implements               |
Dependency        | GUser     |           | GUserRepos |  |  |           +                    |
   +              | Command   +------+----+ Command    |  |  |       +---+--------+           |
   |  +---------> +-----------+      +    +------------+  |  |       | Simple     +----+      |
   |  +                            Uses                   |  |       | Credentials|  Uses     |
   | Creates                         +                    |  |       | Collection |    +      |
   |  +              +--+------------+---+--+-------------+--+       | Flow       |    |      |
   |  |              |  |                |  |                        +-------^----+    |      |
   |  |         +----v--v-+       +------v--v---+    +-------------+         |         | +----+-----+
   |  |         | UserIO  |       | Bitbucket   |    | Credentials |         |         | |Untestable|
   |  |         |         |       | Client      |    | Persistence | <-----------------+ |          |
   |  |         +-^-------+       +-^-----------+    +---^---------+         |         | +----+-----+
   |  |     Implements         Implements          Implements                |         |      |
   |  |           +                 +                    +                   +         |      |
   |  |   +-------+---+       +-----+-------+     +------+-------+   <--+{To UserIO}+--+      |
   |  +-> | ConsoleIO |  +--> | FileBased   |  +> | RestBBClient |           +                +
   |  |   |           |  |    | CredPersist |  |  |              |           |              Uses
   |  |   +-----+-----+  |    +-----+-------+  |  +------+-------+           |                +
   |  +         +        +          +          +         +                   +                |
   | Creates  Uses    Creates     Uses     Creates     Uses               Creates             |
   |  +         +        +          +          +         +                   +                |
   |  |   +-----v----+   |    +-----v------+   |   +-----v-----+             |                |
   |  |   | Console  |   |    | FileSystem |   |   |Bitbucket  |             |                |
   |  |   |          |   |    |            |   |   |           |             |                |
   |  |   +----------+   |    +------------+   |   +-----------+             |                |
   |  |                  |                     |                             |       +--------+-----+
   +--+------------------+---------------------+-----------------------------+-------+ DI Framework |
                                                                                     |              |
                                                                                     +-------^------+
                                                                                             |Drives
                                                                                             |
                                                                                     +-------+-------+
                                                                                     | ConsoleConfig |
                                                                                     |               |
                                                                                     +---------------+

You can tell a lot about a system...

Unit tests are at the same time a forcing function on and an output of software design. You can tell a lot about a system by simply scanning through its tests. If the tests are redundant, hard to follow and lacking of any real organization, there's more than a good chance that the system itself suffers from the same problems. If, on the other hand, the tests are concise and focus on one very specific aspect of the system at a time, this is almost a gaurantee that the system is implemented using sound software design principals. The two go hand in hand.

My hope this project clearly illustrates that point.