How to write unit tests before refactoring?

Question

I've read some answers to questions along a similar line such as "How do you keep your unit tests working when refactoring?". In my case the scenario is slightly different in that I've been given a project to review and bring in line with some standards we have, currently there are no tests at all for the project!

I've identified a number of things I think could have been done better such as NOT mixing DAO type code in a service layer.

Before refactoring it seemed like a good idea to write tests for the existing code. The problem it appears to me is that when I do refactor then those tests will break as I'm changing where certain logic is done and the tests will be written with the previous structure in mind (mocked dependencies etc.)

In my case, what would be the best way to procede? I'm tempted to write the tests around the refactored code but I'm aware there is a risk I may refactor things incorrectly that could change the desired behaviour.

Whether this is a refactor or a redesign I'm happy for my understanding of those terms to be corrected, currently I'm working on the following definition for refactoring "With refactoring, by definition, you don't change what your software does, you change how it does it.". So I'm not changing what the software does I'd be changing how/where it does it.

Equally I can see the argument that if I'm changing the signature of methods that could be considered a redesign.

Here's a brief example

MyDocumentService.java (current)

public class MyDocumentService {
   ...
   public List<Document> findAllDocuments() {
      DataResultSet rs = documentDAO.findAllDocuments();
      List<Document> documents = new ArrayList<>();
      for(DataObject do: rs.getRows()) {
         //get row data create new document add it to 
         //documents list
      }

      return documents;
   }
}

MyDocumentService.java (refactored/redesigned whatever)

public class MyDocumentService {
   ...
   public List<Document> findAllDocuments() {
      //Code dealing with DataResultSet moved back up to DAO
      //DAO now returns a List<Document> instead of a DataResultSet
      return documentDAO.findAllDocuments();
   }
}

Answer 1

You're looking for tests that check for regressions. i.e. breaking some existing behaviour. I would start by identifying at what level that behaviour will remain the same, and that the interface driving that behaviour will remain the same, and start putting in tests at that point.

You now have some tests that will assert that whatever you do below this level, your behaviour remains the same.

You're quite right to question how the tests and code can remain in sync. If your interface to a component remains the same, then you can write a test around this and assert the same conditions for both implementations (as you create the new implementation). If it doesn't, then you have to accept that a test for a redundant component is a redundant test.

Answer 2

The recommended practice is to start with writing "pin-down tests" that test the current behaviour of the code, possibly including bugs, but without requiring you to descend into the madness of discerning whether a given behaviour that violates requirements documents is a bug, workaround for something you're not aware of, or represents an undocumented change in requirements.

It makes the most sense for these pin-down tests to be at a high level, i.e. integration rather than unit tests, so that they'll keep working when you start refactoring.

But some refactorings might be necessary to make the code testable - just be careful to stick to "safe" refactorings. For example, in nearly all cases methods that are private can be made public without breaking anything.

Answer 3

I suggest - if you haven't already - reading both Working Effectively With Legacy Code as well as Refactoring - Improving the Design of Existing Code.

[..] The problem it appears to me is that when I do refactor then those tests will break as I'm changing where certain logic is done and the tests will be written with the previous structure in mind (mocked dependencies etc.) [..]

I don't necessarily see this as a problem: Write the tests, change the structure of your code, and then adjust the test structure also. This will give you direct feedback whether your new structure is actually better than the old one, because if it is, the adjusted tests will be easier to write (and thus changing the tests should be relatively straightforward, lowering the risk of having a newly introduced bug pass the tests).

Also, as others have already written: Don't write too detailed tests (at least not at the beginning). Try to stay at a high level of abstraction (thus your tests will probably better characterized as regression or even integration tests).

Answer 4

Don't write strict unit tests where you mock all the dependencies. Some people will tell you these aren't real unit tests. Ignore them. These tests are useful, and that's what matters.

Let's look at your example:

public class MyDocumentService {
   ...
   public List<Document> findAllDocuments() {
      DataResultSet rs = documentDAO.findAllDocuments();
      List<Document> documents = new ArrayList<>();
      for(DataObject do: rs.getRows()) {
         //get row data create new document add it to 
         //documents list
      }

      return documents;
   }
}

Your test probably looks something like this:

DocumentDao documentDao = Mock.create(DocumentDao.class);
Mock.when(documentDao.findAllDocuments())
    .thenReturn(DataResultSet.create(...))
assertEquals(..., new MyDocumentService(documentDao).findAllDocuments());

Instead of mocking DocumentDao, mock its dependencies:

DocumentDao documentDao = new DocumentDao(db);
Mock.when(db...)
    .thenReturn(...)
assertEquals(..., new MyDocumentService(documentDao).findAllDocuments());

Now, you can move logic from MyDocumentService into DocumentDao without the tests breaking. The tests will show that the functionality is the same (so far as you've tested it).

Answer 5

As you say, if you change the behaviour then it is a transformation and not a refactor. At what level you change the behaviour is what makes the difference.

If there are no formal tests at the highest level then try and find a set of requirements that clients (calling code or humans) that need to stay the same after your redesign for your code to be considered working. That is the list of test cases you need to implement.

To address your question about changing implementations requiring changing test cases I would suggest you take a look at Detroit(classical) vs London(mockist) TDD. Martin Fowler talks about this in his great article Mocks aren't stubs but many people have opinions. If you start at the highest level, where your externals can't change, and work your way down then the requirements should stay fairly stable until you get to a level that really needs to change.

Without any tests this is going to be difficult, and you might want to consider run clients through dual code paths (and recording the differences) until you can be sure your new code does exactly what it needs to do.

Answer 6

Here my approach. It has a cost in terms of time because it's a refactor-test in 4 phases.

What I'm going to expose may suite better in components with more complexity than the one exposed in the question's example.

Anyways the strategy is valid for any component candidate to be normalized by an interface (DAO, Services, Controllers,...).

1. The interface

Lets gather all public methods from MyDocumentService and lets put them all together into a interface. For instance. If it does exist already, use that one instead of setting any new one.

public interface DocumentService {

   List<Document> getAllDocuments();

   //more methods here...
}

Then we force MyDocumentService to implement this new interface.

So far so good. No major changes were made, we respected the current contract and behaivos remains untouched.

public class MyDocumentService implements DocumentService {

 @Override
 public List<Document> getAllDocuments(){
         //legacy code here as it is.
        // with no changes ...
  }
}

2. Unit test of legacy code

Here we have the hard work. To set up a test suite. We should set as many cases as possible: successful cases and also error cases. These last are for the good of the result's quality.

Now, instead of testing MyDocumentService we are going to use the interface as the contract to be tested.

Im not going to go into details, so forgive me If my code looks too simple or too agnostic

public class DocumentServiceTestSuite {

   @Mock
   MyDependencyA mockDepA;

   @Mock
   MyDependencyB mockDepB;

    //... More mocks

   DocumentService service;

  @Before
   public void initService(){
       service = MyDocumentService(mockDepA, mockDepB);
      //this is purposed way to inject 
      //dependencies. Replace it with one you like more.  
   }

   @Test
   public void getAllDocumentsOK(){
         // here I mock depA and depB
         // wanted behaivors...

         List<Document> result = service.getAllDocuments();

          Assert.assertX(result);
          Assert.assertY(result);
           //... As many you think appropiate
    } 
 }

This stage takes longer than any other in this approach. And it's the most important because it will set the point of reference for future comparisions.

Note: Due to no major changes were made and behaivor remains untouched. I suggest to do a tag here into the SCM. Tag or branch doesn't matter. Just do a version.

We want it for rollbacks, versions comparisions and may be for parallel executions of the old code and the new one.

3. Refactoring

Refactor is going to be implemented into a new component. We wont do any change on the existing code. The first step is as easy as to do copy&paste of MyDocumentService and rename it to CustomDocumentService (for example).

New class keep implementing DocumentService. Then go and refactorize getAllDocuments(). (Lets start by one. Pin-refactors)

It may require some changes on DAO's interface/methods. If so, don't change existing code. Implement your own method in DAO interface. Annotate old code as Deprecated and you will know later on what should be removed.

It's important to don't break/change existing implementation. We want to execute both services in parallel and then compare results.

public class CustomDocumentService implements DocumentService {

 @Override
 public List<Document> getAllDocuments(){
         //new code here ...
         //due to im refactoring service 
         //I do the less changes possible on its dependencies (DAO).
         //these changes will come later 
         //and they will have their own tests
  }
 }

4. Updateing DocumentServiceTestSuite

Ok, now the easier part. To add the tests of the new component.

public class DocumentServiceTestSuite {

   @Mock
   MyDependencyA mockDepA;

   @Mock
   MyDependencyB mockDepB;

   DocumentService service;
   DocumentService customService;

  @Before
   public void initService(){
       service = MyDocumentService(mockDepA, mockDepB);
        customService = CustomDocumentService(mockDepA, mockDepB);
       // this is purposed way to inject 
       //dependencies. Replace it with the one you like more
   }

   @Test
   public void getAllDocumentsOK(){
         // here I mock depA and depB
         // wanted behaivors...

         List<Document> oldResult = service.getAllDocuments();

          Assert.assertX(oldResult);
          Assert.assertY(oldResult);
           //... As many you think appropiate

          List<Document> newResult = customService.getAllDocuments();

          Assert.assertX(newResult);
          Assert.assertY(newResult);
           //... The very same made to oldResult

          //this is optional
Assert.assertEquals(oldResult,newResult);
    } 
 }

Now we have oldResult and newResult both validated independently but we can also compare with each other. This last validation is optional and it is dependent to the result. May be it's not comparable.

May not make too much seense to compare two collections in this way, but would be valid for any other kind of object (pojos, data model entities, DTOs, Wrappers, native types...)

Notes

I would not dare to tell how to do unit tests or how to use mock libs. I don't dare neither to say how you have to do the refactor. What I wanted to do is to suggest a global strategy. How to take it forward depends on you. You know exactly how code is, its complexity and if such strategy worth a try. Facts like time and resources matters here. Also matters what do you expect from these tests in the future.

I have started my examples by a Service and I would follow with DAO and so on. Going deep into dependency levels. More or less it could be described as up-bottom strategy. However for minor changes/refactors (like the one exposed in tour example), a bottom up would do the task easier. Because the scope of the changes is little.

Finally, it is up to you to remove deprecated code and to redirect old dependencies to the new one.

Remove also deprecated tests and job is done. If you versioned the old solution with its tests, you can check and compare each other any time.

In consequence of so many work, you have legacy code tested, validated and versioned. And new code, tested, validated and ready to be versioned.

Answer 7

tl;dr Don't write unit tests. Write tests at a more appropriate level.

---

Given your working definition of refactoring:

you don't change what your software does, you change how it does it

there is very wide spectrum. On one end is a self-contained change to particular method, perhaps using a more efficient algorithm. On the other end is porting to another language.

Whatever level of refactoring/redesign is being performed, it is important to have tests that operate at that level or higher.

Automated tests are often classified by level as:

• Unit tests - Individual components (classes, methods)

• Integration tests - Interactions between components

• System tests - The complete application

Write the level of test that can endure the refactoring essentially untouched.

Think:

What essential, publicly-visible behavior will the application have both before and after the refactoring? How can I test that thing still works the same?

Answer 8

Don't waste time writing tests that hook in at points where you can anticipate that the interface is going to change in a non-trivial way. This is often a sign that you are trying to unit-test classes that are 'collaborative' in nature - whose value isn't in what they do themselves, but in how they interact with a number of closely-related classes to produce valuable behaviour. It's that behaviour that you want to test, which means that you want to be testing at a higher level. Testing below this level often requires a lot of ugly mocking, and the resultant tests can be more of a drag on development than an aid to defending behaviour.

Don't get too hung up on whether you're making a refactor, redesign, or whatever. You can make changes that at the lower level constitute a redesign of a number of components, but at a higher integration level simply amount to a refactor. The point is to be clear about what behaviour is of value to you, and defend that behaviour as you go.

It might be useful to consider as you write your tests - could I easily describe to a QA, a product owner, or a user, what this test is actually testing? If it seems like describing the test would be too esoteric and technical, maybe you're testing at the wrong level. Test at the points/levels that 'make sense', and don't gum up your code with tests at every level.

Answer 9

Your first task is to try to come up with the "ideal method signature" for your tests. Strive to make it a pure function. This should be independent of the code that is actually under test; it is a small adapter layer. Write your code to this adapter layer. Now when you refactor your code, you only need to change the adapter layer. Here is a simple example:

[TestMethod]
public void simple_addition()
{
    Assert.AreEqual(7, Eval("3 + 4"));
}

[TestMethod]
public void order_of_operations()
{
    Assert.AreEqual(52, Eval("2 + 5 * 10"));
}

[TestMethod]
public void absolute_value()
{
    Assert.AreEqual(9, Eval("abs(-9)"));
    Assert.AreEqual(5, Eval("abs(5)"));
    Assert.AreEqual(0, Eval("abs(0)"));
}

static object Eval(string expression)
{
    // This is the code under test.
    // I can refactor this as much as I want without changing the tests.
    var settings = new EvaluatorSettings();
    Evaluator.Settings = settings;
    Evaluator.Evaluate(expression);
    return Evaluator.LastResult;
}

The tests are good, but the code under test has a bad API. I can refactor it without changing the tests simply by updating my adapter layer:

static object Eval(string expression)
{
    // After refactoring...
    var settings = new EvaluatorSettings();
    var evaluator = new Evaluator(settings);
    return evaluator.Evaluate(expression);
}

This example seems a pretty obvious thing to do per the Don't Repeat Yourself principle, but it may not be so obvious in other cases. The advantage goes beyond DRY - the real advantage is the decoupling of the tests from the code under test.

Of course, this technique may not be advisable in all situations. For example, there would be no reason to write adapters for POCOs/POJOs because they don't really have an API that could change independently of test code. Also if you're writing a small number of tests, a relatively large adapter layer would probably be wasted effort.