How do you test/change untested and untestable code?

Question

Lately I had to change some code on older systems where not all of the code has unit tests.
Before making the changes I want to write tests, but each class created a lot of dependencies and other anti-patterns which made testing quite hard.
Obviously, I wanted to refactor the code to make it easier to test, write the tests and then change it.
Is this the way you'd do it? Or would you spend a lot of time writing the hard-to-write tests that would be mostly removed after the refactoring will be completed?

Garrettgarrick

Answer 1

First of all, here's a great article with tips on unit testing. Secondly, I found a great way to avoid making tons of changes in old code is to just refactor it a little until you can test it. One easy way to do this is to make private members protected, and then override the protected field.

For example, let's say you have a class that loads some stuff from the database during the constructor. In this case, you can't just override a protected method, but you can extract the DB logic to a protected field and then override it in the test.

public class MyClass {
    public MyClass() {
        // undesirable DB logic
    }
}

becomes

public class MyClass {
    public MyClass() {
        loadFromDB();
    }

    protected void loadFromDB() {
        // undesirable DB logic
    }
}

and then your test looks something like this:

public class MyClassTest {
    public void testSomething() {
        MyClass myClass = new MyClassWrapper();
        // test it
    }

    private static class MyClassWrapper extends MyClass {
        @Override
        protected void loadFromDB() {
            // some mock logic
        }
    }
}

This is somewhat of a bad example, because you could use DBUnit in this case, but I actually did this in a similar case recently because I wanted to test some functionality totally unrelated to the data being loaded, so it was very effective. I've also found such exposing of members to be useful in other similar cases where I need to get rid of some dependency that has been in a class for a long time.

I would recommend against this solution if you are writing a framework though, unless you really don't mind exposing the members to users of your framework.

It's a bit of a hack, but I've found it quite useful.

Cacus

Answer 2

@valters

I disagree with your statement that tests shouldn't break the build. The tests should be an indication that the application doesn't have new bugs introduced for the functionality that is tested (and a found bug is an indication of a missing test).

If tests don't break the build, then you can easily run into the situation where new code breaks the build and it isn't known for a while, even though a test covered it. A failing test should be a red flag that either the test or the code has to be fixed.

Furthermore, allowing the tests to not break the build will cause the failure rate to slowly creep up, to the point where you no longer have a reliable set of regression tests.

If there is a problem with tests breaking too often, it may be an indication that the tests are being written in too fragile a manner (dependence on resources that could change, such as the database without using DB Unit properly, or an external web service that should be mocked), or it may be an indication that there are developers in the team that don't give the tests proper attention.

I firmly believe that a failing test should be fixed ASAP, just as you would fix code that fails to compile ASAP.

Cacus

Answer 3

I am not sure why would you say that unit tests are going be removed once refactoring is completed. Actually your unit-test suite should run after main build (you can create a separate "tests" build, that just runs the unit tests after the main product is built). Then you will immediately see if changes in one piece break the tests in other subsystem. Note it's a bit different than running tests during build (as some may advocate) - some limited testing is useful during build, but usually it's unproductive to "crash" the build just because some unit test happens to fail.

If you are writing Java (chances are), check out http://www.easymock.org/ - may be useful for reducing coupling for the test purposes.

Artificiality

Answer 4

I have read Working Effectively With Legacy Code, and I agree it is very useful for dealing with "untestable" code.

Some techniques only apply to compiled languages (I'm working on "old" PHP apps), but I would say most of the book is applicable to any language.

Refactoring books sometimes assume the code is in semi-ideal or "maintenance aware" state before refactoring, but the systems I work on are less than ideal and were developed as "learn as you go" apps, or as first apps for some technologies used (and I don't blame the initial developers for that, since I'm one of them), so there are no tests at all, and code is sometimes messy. This book addresses this kind of situation, whereas other refactoring books usually don't (well, not to this extent).

I should mention that I haven't received any money from the editor nor author of this book ;), but I found it very interesting, since resources are lacking in the field of legacy code (and particularly in my language, French, but that's another story).

Eoin

Answer 5

So I totally respect the accepted Mike-Stone answer. Because it is better than nothing.

However, I'll offer another alternative.

The reason I don't use the Mike-Stone answer is that ... it is (IMHO)... "create a ~little more technical debt...to deal with a big technical debt item".

Here is my approach. It is a stepping stone.

See below. I've even included some PTSD code snipplets from years gone by. (a hacky stored procedure caller with a string-array). As in, I'm really trying to simulate some duct-taped together code.

/* BEFORE */

public interface IEmployeeManager
{ 
    void addEmployee(string lastname, string firstname, string ssn, DateTime dob);
}

public class EmployeeManager : (implements) IEmployeeManager
{

    /* no dependencies injected, just "new it up" or "use static stuff" */

    public void addEmployee(string lastname, string firstname, string ssn, DateTime dob)
    {
            if(dob.Subtract(DateTime.Now).Months < 16)
            {
                throw new ArgumentOutOfRangeException("Too young");
            }


           /* STATIC CALL, :( */  
    
    
   DatabaseHelper.runStoredProcedureWrapper("dbo.uspEmployeeAdd", new String[] {lastname, firstname, ssn, dob.ToString()};


            /* "new it up". :( */

            new EmailSender.sendEmail("[email protected]", "new employee alert subject", string.format("New Employee Added. (LastName='{0}', FirstName='{1}')", lastname, firstname));

    }

}

/* AFTER */

public interface IEmployeeDataHelperWrapper
{ 
    void addEmployeeToDatabase(string lastname, string firstname, string ssn, DateTime dob);
}

public class EmployeeDataHelperWrapper : (implements) IEmployeeDataHelperWrapper
{
    public void addEmployeeToDatabase(string lastname, string firstname, string ssn, DateTime dob)
    {
        DatabaseHelper.runStoredProcedureWrapper("dbo.uspEmployeeAdd", new String[] {lastname, firstname, ssn, dob.ToString()};
    }
}

and

public interface IEmailSenderWrapper
{ 
    void sendEmail(string to, string subject, string body);
}

public class EmailSenderWrapper : (implements) IEmailSenderWrapper
{
    public void sendEmail(string to, string subject, string body);
    {
        new EmailSender.sendEmail(to, subject, body);
    }
}

and

public interface IEmployeeManager (NO CHANGE)

and (the refactor)

public class EmployeeManager : (implements) IEmployeeManager
{

    private readonly IEmployeeDataHelperWrapper empDataHelper;

    private readonly IEmailSenderWrapper emailSenderWrapper;

    public EmployeeManager() 
    {
            /* here is the stop-gap "trick", this default constructor does a hard coded "new it up" */
            this(new EmployeeDataHelperWrapper(), new EmailSenderWrapper());
    }

    public EmployeeManager(IEmployeeDataHelperWrapper empDataHelper,
                        IEmailSenderWrapper emailSenderWrapper
    )
    {
        /* now you have this new constructor that injects INTERFACES, that can easier be mocked for unit-test code */

        this.empDataHelper = empDataHelper;
        this.emailSenderWrapper = emailSenderWrapper;
    }


    public void addEmployee(string lastname, string firstname, string ssn, DateTime dob)
    {
            if(dob.Subtract(DateTime.Now).Months < 16)
            {
                throw new ArgumentOutOfRangeException("Too young");
            }

            this.empDataHelper.addEmployeeToDatabase(lastname, firstname, ssn, dob);
            

            this.emailSenderWrapper.sendEmail("[email protected]", "new employee alert subject", string.format("New Employee Added. (LastName='{0}', FirstName='{1}')", lastname, firstname));

    }

}

Essentially, I MOVE code (I do not really "re-code it").

and the risk is small (IMHO).... but "sequentially", its the same code that runs in the same exact order.

Is it perfect? No. But you start thinking about "injected dependencies"... and then they become independently refactor-able.

Marijuana