Chapter 10 I Can’t Run This Method in a Test Harne

Chapter 10: I Can’t Run This Method in a Test Harness

Hidden Methods (testing methods behind access protection)

We need to make a change to a method, but it’s a private method. What should we do?

Can we test it through a public method? Testing through a public method more closely approximates a private methods usage in production. We should always prefer testing through public methods if possible.

This can be made challenging in classes with too many responsibilities. In this case, testing those public methods is already challenging enough, so the idea of testing private methods in isolation becomes more appealing, as it can help facilitate refactoring.

So how do we write a test for a private method? Well the obvious answer to that is to make it public.

❓ There is a point here that says that “if making it (a private method) public bothers us, in most cases, it means our class is doing too much”. I’m not sure if I 100% agree with this. Making a private method public muddies the public interface of a class, although if you’re in this situation in the first place, that public interface is probably already pretty muddy.

Here are some reasons why making a private method public might bother us:

• The method is just a utility; the client shouldn’t have to care about it; it’s implementation details.
• If the method is called at the wrong time in production code, it could have unexpected results.

In aid of testability, the first reason isn’t something we should concern ourselves with too much, although it’s worth considering whether the method would belong better in another class.

The second reason is a bit more serious, although again, the solution here is to move the method out into a separate class, and make it public. The original usage can then create an instance internally.

If there aren’t many tests in place already however, we may want to be a bit more careful with our changes before we start to break things down.

An example:

We have a class declaration in C#

public class CCAImage
{
    public void Snap()
    {
        ...
    }
 
    private void SetSnapRegion(int x, int y, int dx, int dy)
    {
        ...
    }
}

This class is used to take screenshots. One might wonder why an image class is snapping screenshots, but this is legacy code, remember?

We’ve just found out the API for screenshots has changed, and so we need to make a change to SetSnapRegion.

Unfortunately SetSnapRegion manipulates some important private fields that affect the normal running order of CCImage, so if we made it public and it was called at the wrong time, it could have unexpected results.

As was alluded to before, the main reason this class has become untestable is that it has too many responsibilities. Ideally, we would like to break it down into smaller classes, but the decision to do that is affected by things like:

• Where we are in our release cycle
• How much time we have
• Associated risks with such an invasive refactoring (lack of tests, high complexity)

If we decide we won’t do the full refactoring to separate responsibilities right now, can we still write tests? Yes, here’s how.

Step 1: change SetSnapRegion from private to protected.

public class CCAImage
{
    public void Snap()
    {
        ...
    }
 
    protected void SetSnapRegion(int x, int y, int dx, int dy)
    {
        ...
    }
}

This still prevents the method from being called out of order unintentionally in production code, but the method can be accessed in subclasses.

Step 2: we can subclass CCAImage to get access to the method.

public class TestingCCAImage : CCAImage
{
    public void SetSnapRegion(int x, int y, int dx, int dy)
    {
        // call SetSnapRegion on the base class
        base.SetSnapRegion(x, y, dx, dy);
    }
}

With that done, we can call SetSnapRegion in a test, albeit indirectly. But is this approach a good idea? We expressed hesitance earlier around making the method public, but we’ve just done something similar. We’ve made it more accessible, and broken encapsulation a bit, but the change was calculated. The tests we can now write will allow us to come back and do the bigger refactor that we wanted to do initially, and the broken encapsulation is just further encouragement to do so.

“Helpful” unhelpful language features

Some language features might have seemed like a good idea at the time, but in reality tend to skew more towards getting in the way.

Here is a piece of C# code that accepts a collection of uploaded files from a web client. The code iterates through each of them and returns a list of streams associated with files that have particular characteristics.

public void IList GetKSRStreams(HttpFileCollection files)
{
    ArrayList list = new ArrayList();
    foreach(string name in files)
    {
        HttpPostedFile file = files[name];
        if (file.FileName.EndsWith(".ksr") ||
                (file.FileName.EndsWith(".txt")
                            && file.ContentLength > MIN_LEN)) {
            ...
            list.Add(file.InputStream);
        }
    }
    return list;
}

We’d like to make some changes to this piece of code, and maybe even refactor it, but writing tests will be difficult. In order to call GetKSRStreams we need to pass in a HttpFileCollection object and populate it with HttpPostedFile objects, but we can’t do this easily in a test because:

• HttpPostedFile doesn’t have a public constructor
• The class is sealed, which in C# means that we can’t create an instance of it OR subclass it.

HttpPostedFile is part of the .NET library. At runtime, some other class creates instances of this class, but we don’t have access to it. The same problems are also present for HttpFileCollection. There are clear security reasons for doing this, but it’s pretty drastic all the same.

So how do we write tests here? We can’t use Extract Interface or Extract Implementer; the HttpPostedFile and HttpFileCollection classes aren’t under our control, being library classes, we have no ability to change them. The only technique we can use here is Adapt Parameter.

Lucky for us, in our code, all we’re doing is iterating over the collection. Also lucky for us, is that the sealed HttpFileCollection class that our code uses has an unsealed base class named NameObjectCollectionBase. We can subclass it and pass an object of that subclass to GetKSRStreams. The change is perfectly safe and easy if we lean on the compiler.

public void IList GetKSRStreams(OurHttpFileCollection files)
{
    ArrayList list = new ArrayList();
    foreach(string name in files)
    {
        HttpPostedFile file = files[name];
        if (file.FileName.EndsWith(".ksr") ||
                (file.FileName.EndsWith(".txt")
                    && file.ContentLength > MAX_LEN))
        {
            ...
            list.Add(file.InputStream);
        }
    }
    return list;
}

This gets us past the first problem, but the next problem is more difficult. We need HttpPostedFiles to run GetKSRStreams in a test, but we can’t create them. What do we need from them? Based on the code above, we can see that we need a class with a couple of properties: FileName and ContentLength.

We can use Skin and Wrap the API to get some separation between us and the HttpPostedFile class. To do that, we extract an interface (IHttpPostedFile) and write a wrapper (HttpPostedFileWrapper):

public class HttpPostedFileWrapper : IHttpPostedFile
{
    public HttpPostedFileWrapper(HttpPostedFile file) {
        this.file = file;
    }
 
    public int ContentLength {
        get { return file.ContentLength; }
    }
    ...
}

Now that we have an interface, we can create a fake for our tests:

public class FakeHttpPostedFile : IHttpPostedFile
{
    public FakeHttpPostedFile(int length, Stream stream, ...) { ... }
 
    public int ContentLength {
        get { return length; }
    }
}

Now, if we Lean on the Compiler and change our production code, we can use HttpPostedFileWrapper objects or FakeHttpPostedFile objects through the IHttpPostedFile interface without knowing which is being used.

public IList getKSRStreams(OurHttpFileCollection)
{
    ArrayList list = new ArrayList();
    foreach(string name in files)
    {
        IHttpPostedFile file = files[name];
        if (file.FileName.EndsWith(".ksr") ||
                (file.FileName.EndsWith(".txt"))
                    && file.ContentLength > MAX_LEN))
        {
            ...
            list.Add(file.InputStream);
        }
    }
    return list;
}

The only annoyance is that we now have to iterate the original HttpFileCollection in the production code, wrap each HttpPostedFile it contains, nd add it to a new collection that we pass into GetKSRStreams. This is the price of security.

It’s easy to look at the sealed and final keywords and just think they were a huge mistake, and should never been added to languages. The real fault lies with us, for depending directly on libraries that are out of our control.

❓ Before depending directly on a concrete class you don’t own, consider if there is an interface that could be used in its place. Alternatively, wrapping as seen above is a great way to open up restrictive library code for testing.

Undetectable Side Effect

A side effect is when a method does something non-obvious, and gives the caller no way of knowing about that thing without looking through the source.

Clean Code spoke at some length about why side effects are bad. In short, it makes a system harder to understand by having behaviour hidden away in unexpected places. The reality is that most codebases have side effects lying around.

Here is a class with this problem:

public class AccountDetailFrame extends Frame
            implements ActionListener, WindowListener
{
    private TextField display = new TextField(10);
    ...
    public AccountDetailFrame(...) { ... }
 
    public void actionPerformed(ActionEvent event) {
        String source = (String)event.getActionCommand();
        if (source.equals("project activity")) {
            detailDisplay = new DetailFrame();
            detailDisplay.setDescription(
                    getDetailText() + " " + getProjectionText());
            detailDisplay.show();
            String accountDescription
                    = detailDisplay.getAccountSymbol();
            accountDescription += ": ";
            ...
            display.setText(accountDescription);
            ...
        }
    }
    ...
}

It’s unclear what exactly is this classes primary responsibility is. It’s creating GUI elements, receiving actions from them using the actionPerformed handler, and decides what it needs to display and displays it. The way it does this is also a bit weird: It builds up a separate window with detailed text and then shows it. When that window is done with its work, it grabs information directly from it, does a bit of finessing with it, and then sets it to one of its own fields.

Running this in a test harness would be pointless. It would create a window, show it to us, prompt us for some input, and then display something in another window. There is no obvious way to sense what this code does.

❓ This last sentence is a little unclear to me. My interpretation is that he’s saying the side effects make writing meaningful test assertions incredibly difficult.

So what can we do? First, we can start to separate work that is independent of the GUI from work that is dependent on the GUI. We’re working in Java, so we have a suite of refactoring tools available to us. Our first move is to perform a set of Extract Method refactorings to divide up the work in this method.

But where do we start?

The method itself is primarily a hook for notifications from the windowing framework. The first thing it does is get the name of a command from the action event that is passed to it. If we extract the whole body of the method, we can separate ourselves from any dependency on the ActionEvent class.

public class AccountDetailFrame extends Frame
            implements ActionListener, WindowListener
{
    private TextField display = new TextField(10);
    ...
    public AccountDetailFrame(...) { ... }
 
    public void actionPerformed(ActionEvent event) {
        String source = (String)event.getActionCommand();
        performCommand(source);
    }
 
    public void performCommand(String source) {
        if (source.equals("project activity")) {
            detailDisplay = new DetailFrame();
            detailDisplay.setDescription(
                    getDetailText() + " " + getProjectionText());
            detailDisplay.show();
            String accountDescription
                    = detailDisplay.getAccountSymbol();
            accountDescription += ": ";
            ...
            display.setText(accountDescription);
            ...
        }
    }
    ...
}

The code still isn’t testable however. The next step is to extract methods for the code that accesses the other frame. It will help to make the detailDisplay frame an instance variable of the class.

public class AccountDetailFrame extends Frame
            implements ActionListener, WindowListener
{
    private TextField display = new TextField(10);
    private DetailFrame detailDisplay;
    ...
    public AccountDetailFrame(...) { .. }
 
    public void actionPerformed(ActionEvent event) {
        String source = (String)event.getActionCommand();
        performCommand(source);
    }
 
    public void performCommand(String source) {
        if (source.equals("project activity")) {
            detailDisplay = new DetailFrame();
            detailDisplay.setDescription(
                    getDetailText() + " " + getProjectionText());
            detailDisplay.show();
            String accountDescription
                    = detailDisplay.getAccountSymbol();
            accountDescription += ": ";
            ...
            display.setText(accountDescription);
            ...
        }
    }
    ...
}

Now we can extract the code that uses that frame into a set of methods. What should we name the methods? To get ideas for names, we should take a look at what each piece of code does from the perspective of this class, or what it calculates for this class. In addition, we should not use names that deal with the display components. We can use display components in the code that we extract, but the names should hide that fact. With these things in mind, we can create either a command method or a query method for each chunk of code.

Command/Query Separation

Command/Query Separation (Bertrand Meyer): a method should be either a command or a query, never both. A command may modify the object’s state but returns nothing; a query returns a value but doesn’t modify the object. The main payoff is communication — if a method is a query, you shouldn’t have to read its body to know whether calling it twice in a row is safe.

Here’s what the performCommand method looks like after a series of extractions:

public class AccountDetailFrame extends Frame
            implements ActionListener, WindowListener
{
    public void performCommand(String source) {
        if (source.equals("project activity")) {
            setDescription(getDetailText() + " " + getProjectionText());
            ...
            String accountDescription = getAccountSymbol();
            accountDescription += ": ";
            ...
            display.setText(accountDescription);
            ...
        }
    }
 
    void setDescription(String description) {
        detailDisplay = new DetailFrame();
        detailDisplay.setDescription(description);
        detailDisplay.show();
    }
 
    String getAccountSymbol() {
        return detailDisplay.getAccountSymbol();
    }
    ...
}

Now that we’ve extracted all of the code that deals with the detailDisplay frame, we can go through and extract the code that accesses components on the AccountDetailFrame.

public class AccountDetailFrame extends Frame
    implements ActionListener, WindowListener {
    public void performCommand(String source) {
        if (source.equals("project activity")) {
            setDescription(getDetailText() + " " + getProjectionText());
            ...
            String accountDescription
                    = detailDisplay.getAccountSymbol();
            accountDescription += ": ";
            ...
            setDisplayText(accountDescription);
            ...
        }
    }
 
    void setDescription(String description) {
        detailDisplay = new DetailFrame();
        detailDisplay.setDescription(description);
        detailDisplay.show();
    }
 
    String getAccountSymbol() {
        return detailDisplay.getAccountSymbol();
    }
 
    void setDisplayText(String description) {
        display.setText(description);
    }
    ...
}

After those extractions, we can use Subclass and Override Method and test whatever code is left in the performCommand method. For example, if we subclass AccountDetailFrame like this, we can verify that given the "project activity"command, the display gets the proper text:

public class TestingAccountDetailFrame extends AccountDetailFrame
{
    String displayText = "";
    String accountSymbol = "";
 
    void setDescription(String description) {
    }
 
    String getAccountSymbol() {
        return accountSymbol;
    }
 
    void setDisplayText(String text) {
        displayText = text;
    }
}

Here is a test that exercises the performCommandmethod:

public void testPerformCommand() {
    TestingAccountDetailFrame frame = new TestingAccountDetailFrame();
    frame.accountSymbol = "SYM";
    frame.performCommand("project activity");
    assertEquals("SYM: basic account", frame.displayText);
}

When we separate out dependencies this way, very conservatively, by doing automated extracting method refactorings, we might end up with code that makes us flinch a bit. For instance, a setDescription method that creates a frame and shows it is downright nasty. What happens if we call it twice? We have to deal with that issue, but doing these coarse extractions is a decent first step. Afterward, we can see if we can relocate the frame creation to a better place.

Where are we now? We started with a single class whose one important method was performCommand (originally actionPerformed). After the extractions, AccountDetailFrame now has a cluster of small command/query methods (setDescription, getAccountSymbol, setDisplayText, …) that performCommand delegates to.

We can’t really see this in a UML diagram, but getAccountSymbol and setDescription use the detailDisplay field and nothing else. The setDisplayText method uses only the TextFieldnamed display. We could recognise these as separate responsibilities and eventually push the detailDisplay-related methods out into their own collaborator (here called SymbolSource), leaving AccountDetailFrame thinner.

This is an extremely crude refactoring of the original code, but at least it separates responsibilities somewhat. The AccountDetailFrame is tied to the GUI (it is a subclass of Frame) and it still contains business logic. With further refactoring, we can move beyond that, but at the very least, now we can run the method that contained business logic in a test case. It is a positive step forward.

The SymbolSource class is a concrete class that represents the decision to create another Frame and get information from it. However, we named it SymbolSource here because, from the point of view of the AccountDetailFrame, its job is to get symbol information any way it needs to. I wouldn’t be surprised to see SymbolSource become an interface, if that decision ever changes.

The steps we took in this example are very common. When we have a refactoring tool, we can easily extract methods on a class and then start to identify groups of methods that can be moved to new classes. A good refactoring tool will only allow you to do an automated extract method refactoring when it is safe. However, that just makes the editing that we do between uses of the tool the most hazardous part of the work. Remember that it is okay to extract methods with poor names or poor structure to get tests in place. Safety first. After the tests are in place, you can make the code much cleaner.

Source

Working Effectively with Legacy Code, Michael Feathers — Chapter 10, I Can’t Run This Method in a Test Harness.