Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I wrote some sorting algorithms for a class assignment and I also wrote a few tests to make sure the algorithms were implemented correctly. My tests are only like 10 lines long and there are 3 of them but only 1 line changes between the 3 so there is a lot of repeated code. Is it better to refactor this code into another method that is then called from each test? Wouldn't I then need to write another test to test the refactoring? Some of the variables can even be moved up to the class level. Should testing classes and methods follow the same rules as regular classes/methods?

Here's an example:

    [TestMethod]
    public void MergeSortAssertArrayIsSorted()
    {
        int[] a = new int[1000];
        Random rand = new Random(DateTime.Now.Millisecond);
        for(int i = 0; i < a.Length; i++)
        {
            a[i] = rand.Next(Int16.MaxValue);
        }
        int[] b = new int[1000];
        a.CopyTo(b, 0);
        List<int> temp = b.ToList();
        temp.Sort();
        b = temp.ToArray();

        MergeSort merge = new MergeSort();
        merge.mergeSort(a, 0, a.Length - 1);
        CollectionAssert.AreEqual(a, b);
    }
    [TestMethod]
    public void InsertionSortAssertArrayIsSorted()
    {
        int[] a = new int[1000];
        Random rand = new Random(DateTime.Now.Millisecond);
        for (int i = 0; i < a.Length; i++)
        {
            a[i] = rand.Next(Int16.MaxValue);
        }
        int[] b = new int[1000];
        a.CopyTo(b, 0);
        List<int> temp = b.ToList();
        temp.Sort();
        b = temp.ToArray();

        InsertionSort merge = new InsertionSort();
        merge.insertionSort(a);
        CollectionAssert.AreEqual(a, b); 
    }
share|improve this question

5 Answers 5

up vote 18 down vote accepted

Test code is still code and also needs to be maintained.

If you need to change the copied logic, you need to do that in every place you copied it to, normally.

DRY still applies.

Wouldn't I then need to write another test to test the refactoring?

Would you? And how do you know the tests you currently have are correct?

You test the refactoring by running the tests. They should all have the same results.

share|improve this answer
    
Right on. Tests are code -- all the same principles for writing good code still apply! Test the refactoring by running the tests, but be sure that there is adequate coverage and that yo're hitting more than one boundary condition in you tests (e.g. a normal condition vs. a failure condition). –  Michael Mar 18 '12 at 22:47
5  
I disagree. Tests don't necessarily have to be DRY, it's more important for them to be DAMP (Descriptive And Meaningful Phrases) than DRY. (In general, at least. In this specific case though, pulling out the repeated initialization into a helper definitely makes sense.) –  Jörg W Mittag Mar 19 '12 at 4:21
1  
I've never heard DAMP before, but I like that description. –  Joachim Sauer Apr 20 '12 at 7:33
    
@Jörg W Mittag: You can still be DRY and DAMP with tests. I usually refactor the different ARRANGE-ACT-ASSERT (or GIVEN-WHEN-THEN) parts of the test to helper methods in the test fixture if I know that some part of the test repeats. They usually have DAMP names, such as givenThereAreProductsSet(amount) and even as simple as actWith(param). I managed to do it with fluent api wise (e.g. givenThereAre(2).products()) once, but I quickly stopped because it felt like an overkill. –  Spoike Apr 20 '12 at 8:19

Duplicating code for tests is an easy trap to fall into. Sure it's convenient, but what happens if you start to refactor your implementation code and your tests all start needing to change? You run the same risks that you do if you've duplicated your implementation code, in that you will most likely need to change your test code in many places also. This all adds up to a great deal of wasted time, and an increasing number of failure points that need to be dealt with, which means that the cost to maintain your software becomes unnecessarily high, and therefore reduces the overall business value of the software you work on.

Consider also that what is easy to do in tests will become easy to do in the implementation. When you are pressed for time and under a lot of stress, people tend to rely on learned patterns of behavior and generally try and do what seems easiest at the time. So, if you find you cut and paste a lot of your test code, it's likely you'll be doing the same in your implementation code, and this is a habit you want to avoid early on in your career, to save you a lot of difficulty later on when you find yourself having to maintain older code that you have written, and that your company can't necessarily afford to rewrite.

As others have said, you apply the DRY principal, and you look for opportunities to refactor any likely duplications to helper methods and helper classes, and yes, you should even be doing this in your tests in order to maximize the reuse of code and save yourself facing difficulties with maintenance later on. You may even find yourself slowly developing a testing API that you can use over and over again, possibly even in multiple projects - certainly that is how things have happened for me over the last several years.

share|improve this answer

No, it's not OK. You should use a TestDataBuilder instead. You should also take care about readability of your tests : a ? 1000 ? b ? If tomorrow one have to work on the implementation you are testing, tests are a great way to enter the logic : write your tests for you fellow programmers, not for the compiler :)

Here are your tests implementation, "revamped" :

/**
* Data your tests will exercice on
*/
public class MyTestData(){
    final int [] values;
    public MyTestData(int sampleSize){
        values = new int[sampleSize];
        //Out of scope of your question : Random IS a depencency you should manage
        Random rand = new Random(DateTime.Now.Millisecond);
        for (int i = 0; i < a.Length; i++)
        {
            a[i] = rand.Next(Int16.MaxValue);
        }
    }
    public int [] values();
        return values;
    }

}

/**
* Data builder, with default value. 
*/
public class MyTestDataBuilder {
    //1000 is actually your sample size : emphasis on the variable name
    private int sampleSize = 1000; //default value of the sample zie
    public MyTestDataBuilder(){
        //nope
    }
    //this is method if you need to test with another sample size
    public MyTestDataBuilder withSampleSizeOf(int size){
        sampleSize=size;
    }

    //call to get an actual MyTestData instance
    public MyTestData build(){
        return new MyTestData(sampleSize);
    }
}

public class MergeSortTest { 

    /**
    * Helper method build your expected data
    */
    private int [] getExpectedData(int [] source){
        int[] expectedData =  Arrays.copyOf(source,source.length);
        Arrays.sort(expectedData);
        return expectedData;
    }
}

//revamped tests method Merge
    public void MergeSortAssertArrayIsSorted(){
        int [] actualData = new MyTestDataBuilder().build();
        int [] expected = getExpectedData(actualData);
        //Don't know what 0 is for. An option, that should have a explicit name for sure :)
        MergeSort merge = new MergeSort();
        merge.mergeSort(actualData,0,actualData.length-1); 
        CollectionAssert.AreEqual(actualData, expected);
    }

 //revamped tests method Insertion
 public void InsertionSortAssertArrayIsSorted()
    {
        int [] actualData = new MyTestDataBuilder().build();
        int [] expected = getExpectedData(actualData);
        InsertionSort merge = new InsertionSort();
        merge.insertionSort(actualData);
        CollectionAssert.AreEqual(actualData, expectedData); 
    }
//another Test, for which very small sample size matter
public void doNotCrashesWithEmptyArray()
    {
        int [] actualData = new MyTestDataBuilder().withSampleSizeOf(0).build();
        int [] expected = getExpectedData(actualData);
        //continue ...
    }
}
share|improve this answer

Even more than production code, test code needs to be optimized for readability and maintainability, as it has to be maintained along the code being tested and also be read as part of the documentation. Consider how copied code may make test code maintenance harder and how that may become an incentive not to write tests for everything. Also, don't forget that when you write a function to DRY your tests, it also should be subject to tests.

share|improve this answer

As Oded already said, test code still needs to be maintained. I'd add that repetition in test code makes it harder for maintainers to understand the structure of tests, and to add new tests.

In the two functions you posted, the following lines are absolutely identical except for one space difference at the start of the for loop:

        int[] a = new int[1000];
        Random rand = new Random(DateTime.Now.Millisecond);
        for (int i = 0; i < a.Length; i++)
        {
            a[i] = rand.Next(Int16.MaxValue);
        }
        int[] b = new int[1000];
        a.CopyTo(b, 0);
        List<int> temp = b.ToList();
        temp.Sort();
        b = temp.ToArray();

This would be a perfect candidate for moving in to some kind of helper function, whose name indicates that it is initialising data.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.