Test anti-patterns project, contributors wanted!

Take a look at the following test class. I deliberately wrote this poor quality test, so that I could show how easy it is to re-factor it to a better one. I was driven by having seen lots of such poor tests and to be honest I don’t want to see another of its ilk again. I might write a test like this myself, but I would never leave it like this. Its part finished. Its littered with cut and paste, poorly named methods and hard coded values. It’s a bit like a surgeon performing an operation and not sewing up the hole! I would go as far as to say that leaving tests in this state is unprofessional. There is no excuse as most of the lessons of good naming and code re-use have been written down a long time back. More lately in Clean Code by Robert C. Martin. I just don’t understand why people still write rubbish like this! Possibly because they are allowed to get away with it! Note that the test provides 100% coverage of the implementation class, but its not enough to stop there.

Have a read over the class, and then move on to read the steps to re-factor to better patterns. I have several designs here that focus on the setup stage of a test. As your tests get more complex you will find the patterns here support ever increasing complexity. In an Agile project you might move from one system to the next as the requirements ramp up. Then once you see where I am heading, I lay down a challenge.

Test anti-patterns project, contributors wanted!

If you have a Test Anti-Pattern that you would like to showcase. Check out my code from gitHub, add the new example and examples of how to re-factor the code. Blog it to your own blog with a reference to this page, or drop a comment. If we get enough I will setup a page with an index to all the blogs.

The code can be forked out with git from gitHub http://github.com/mhgit/TerribleJavaTestingMadeGood. It should be an easy start. Its a full maven project with a small dependency set and maven site reports already built in for test coverage. Its fully open source with the Apache 2.0 license. Once you have your new example, tell me about it by adding a comment to this page, and following the git forking instructions for making a git pull available.

I hope I never see another badly written test like this:

…but unfortunately I probably will…

package org.testpatterns.dupcodeexample.lineutil.unit;

  import org.testpatterns.dupcodeexample.lineutil.SimplePage;
  import java.util.HashMap;
  import java.util.Map;
  import java.util.Set;
  import org.junit.Test;
  import static org.junit.Assert.*;

  /**
  * Terrible Test SimplePage Function.
  *
  * @author martinh
  */
  public class TerribleSimplePageTest {

    @Test
    public void testPageAssembley() {

        String item = "-23456789-";

        Map<Integer, String> expectedLines = new HashMap<Integer, String>();

        expectedLines.put(1, item + item + item + item + item);
        expectedLines.put(2, item + item + item + item + item);
        expectedLines.put(3, item + item + item + item + item);
        expectedLines.put(4, item + item + item + item + item);

        SimplePage page = SimplePage.newInstance(item, 20);
        Map<Integer, String> actualMap = page.getMap();

        assertEquals(expectedLines, actualMap);

    }

    @Test
    public void testPageKeySet() {

        String item = "-23456789-";

        Map<Integer, String> expectedLines = new HashMap<Integer, String>();

        expectedLines.put(1, item + item + item + item + item);
        expectedLines.put(2, item + item + item + item + item);
        expectedLines.put(3, item + item + item + item + item);
        expectedLines.put(4, item + item + item + item + item);

        SimplePage page = SimplePage.newInstance(item, 20);
        Set<Integer> actualKeySet = page.getKeySet();

        assertEquals(expectedLines.keySet(), actualKeySet);

    }

    @Test
    public void testAddLines() {

        String item = "-23456789-";

        SimplePage page = SimplePage.newInstance(item, 20);

        Map<Integer, String> additionalLines = new HashMap<Integer, String>();

        additionalLines.put(1, item + item + item + item + item);
        additionalLines.put(2, item + item + item + item + item);
        additionalLines.put(3, item + item + item + item + item);
        additionalLines.put(4, item + item + item + item + item);

        page.appendLines(item, 20);

        Map<Integer, String> actualMap = page.getMap();

        Map<Integer, String> expectedLines = new HashMap<Integer, String>();

        expectedLines.put(1, item + item + item + item + item);
        expectedLines.put(2, item + item + item + item + item);
        expectedLines.put(3, item + item + item + item + item);
        expectedLines.put(4, item + item + item + item + item);
        expectedLines.put(5, item + item + item + item + item);
        expectedLines.put(6, item + item + item + item + item);
        expectedLines.put(7, item + item + item + item + item);
        expectedLines.put(8, item + item + item + item + item);
        assertEquals(expectedLines, actualMap);

    }
}

Fixture as a normal method: ImprovedSimplePageTest

I am not going to post up code for all the classes. Check it out from gitHub for that. So I just highlight the main steps and show the final full class.

The basis of the first pattern is simple. Most small tests like this just need one method for setup, this eliminates all the cut and paste. This might be run from an @Before, or @BeforeClass annotation, as part of the constructor or perhaps just from the test itself. These techniques will have to be applied depending on what you need to achieve. For instance if your setup is inserting into a database, you have to make sure your data does not violate db constraints. So the annotations might not help, plus you have the added complexity of getting hold of your database. A good pattern is to use an in memory db, but some of the annotations will cause the setup to get run before each test. So in a nutshell attempt to have one setup, and its not that important which system you use to run it, that will depend on what your doing.

Refactoring steps:

Rename methods.

Have a good strategy for your test names if they are good enough you will not need to comment the methods. The pattern I use here is: [What is being tested]_[What is expected]. If you can’t fit it all into the name, the test is probably too big and needs breaking down. A test should be concise.

Remove duplication.

I Added this method: createLineExpectations to create expectations.

This allowed me to remove all the duplicated test setup that had been cut and pasted all through the class. I wrote this bad test, using TDD to come up with the supporting implementation, so even as I wrote it the test went through several iterations. During that process I left a chunk of unused map behind. Which just goes to prove that if your test is messy, its difficult to spot problems.
An unused chunk of code that hid in the duplications

  Map<Integer, String> additionalLines = new HashMap<Integer, String>();

  additionalLines.put(1, item + item + item + item + item);
  additionalLines.put(2, item + item + item + item + item);
  additionalLines.put(3, item + item + item + item + item);
  additionalLines.put(4, item + item + item + item + item);

Added constants and moved all the hard coded value out.

Collecting setup into an inner fixture class: LocalFixtureBasedPageTest

Its not long before the technique starts to breakdown. As you add more supporting methods it gets harder to see what is test, and what is setup. So the next stage is to move the setup into an inner class. This way, most ide’s will allow you to collapse the fixture hiding it away. When I read a test, I am mostly interested in the expectations, not the setup. Note that the constants have been moved into the fixture. So the fixture becomes an inner class that supplies everything to do with setup for the test.

    private class DataFixture {

        private final String ITEM = "-23456789-";
        private final int LINES_8 = 8;
        private final int LINES_4 = 4;

        Map<Integer, String> createLineExpectations(String item,
                int numLinesToCreate) {
            Map<Integer, String> expectedLines = Maps.newHashMap();

            StringBuilder sb = new StringBuilder();
            sb.append(item).append(item).append(item).append(item).append(item);
            String line = sb.toString();
            for (int i = 0; i < numLinesToCreate; i++) {
                    expectedLines.put(i + 1, line);
            }

            return expectedLines;
        }
    }

Fixture as a spring injected component: SpringWiredSimplePageTest

When a test gets more complex you sometimes end up with masses of complexity building up in inner classes. I sometimes use inner classes to build complex data requirements using the setter method chaining pattern for instance. Each inner class becoming a container for parameterized setup. The next level of complexity is needing to share the setup over many test classes.

These are your options.

Create a static helper class

Not a bad solution, but I hate having to access the methods via the class name. Static classes often collect unrelated methods too. Also, its often not possible to work with the restriction of being static.

Abstractions and class hierarchy.

This is often posed as a solution when you need to share the setup over many classes. In this case its a terrible solution. The abstraction gets further complicated because your using the wrong OO mechanism. It also means your test has to extend a class, in some situations, you are already extending to get at the internals of the object under test.

Use method injection to supply a fixture component.

If you break the fixture out to a component its easier to manage the complexity of the fixture away from the test with composition. You can have many components each doing something specific for a set of tests. You could even inject more than one component. The design of the fixture can use other OO patterns to keep it neat. For instance I once built a set of fixtures to supply ready made folder and file structures. They all extended an abstract and implemented an interface. The @Qualifier annotation was used to inject the correct implementation into the test. On occasion a test used two of the component implementations.

Refactoring steps:

Move the Fixture inner class to its own component class. If you get many of these its worth putting them in a separate project.
Add the @Component annotation from the spring project.
Change the type giving a more descriptive name.
Make the methods in the fixture public.
Better names for methods and types now that the fixture is not near the code that uses it.
Java doc for important methods.
Addition of getters for fields instead of direct access.
Improved createLineExpectations(int numLinesToCreate) by removing the dataItem, the fixture knows this data so no need to supply via method.
Convert to spring test. This means its possible to load in the spring configuration and autowire the test

The test class markup to convert it into a spring test, and provide amongst other things auto wiring:

@RunWith(SpringJUnit4ClassRunner.class)
@ContextConfiguration(locations = "classpath:springAppConfig.xml")
@TestExecutionListeners({DependencyInjectionTestExecutionListener.class})
public class SpringWiredSimplePageTest {

Autowire the fixture, and spring will inject it:

@Autowired
    private PageDataFixture dataFixture;

Your test setup is then just called like this:

Map<Integer, String> expectedLines = dataFixture.createPageExpectation(NUMBER_LINES_ON_PAGE_4, NUMBER_DATA_ITEMS_IN_LINE_5);

Conclusions

Once you have a test that gives 100% coverage, there is usually some more time that needs to be spent on cleaning the code. There are some easy patterns that can help, there is nothing in this article that is complex or difficult to understand so there is no excuse for leaving it messy. Bugs hide in messy code and I think you would agree, the final result is very easy to read:SpringWiredSimplePageTest