Test cases do all the work through helper method -- bad practice?

Question

Consider a test suite like this:

public class MyTestSuite {
   @Test
   public void test_something() {
      testHelperMethod("value11", "value12", "value13");
   }

   @Test
   public void test_something_else_meaningful_name() {
      testHelperMethod("othervalue21", "othervalue22", "othervalue23");
   }

   // ...

   private void testHelperMethod(String value1, String value2, String value3) {
       // set up part (independent of value1, value2, value3)

       // action part (dependent on values)

       // assertion part (dependent on values)

       // tear down part (independent of values)
   }
}

In other words, all the test cases are executed via a single, parametrized helper method, which is structured according to the arrange-act-assert schema. The test cases just call this method with various parameters, according to what exactly needs to be tested.

Question: what is the disadvantage, if any, of structuring the tests like this, and does this (anti?)-pattern have a common name?

Remarks

1. I tried to Google for it for a long time, including on SO and on the blog, but could not find anything useful till now.

2. This question is the closest I found, but the answers there address other aspects/problems, namely:

   • assertions/setup code intermixed (not the case here, as all assertions are at the end of the helper method)
   • more assertions per test (also the case in my example, but I think this is an unrelated issue)
   • responsibility of helper method is not clear: I think in my case it is clear, just it is different from the 'traditional' one

3. To be more precise, the assertion part in testHelperMethod is also a separate utility method (called with many-many parameters), but I guess that does not matter much for the question. (I.e.: why is it bad practice to delegate testing to helper methods, if at all?)

4. In case this matters, this is an integration test, but I suspect that the answer would be the same for unit-tests as well.

EDIT: Changed the names of the test cases to avoid confusion (Previously, they were called test1, test2. In fact, the tests in the project in questions do have meaningful names, I had just made this simplification myself.)

Answer 1

As others have already stated: nothing wrong with breaking out a separate method to avoid duplication.

However, as others have also stated: you do want each individual test to clearly indicate what it is setting up and what it is asserting.

Test names will help with that enormously and I assume your actual code has more meaningful names than Test1 and Test2.

To make the code of the individual tests more "intelligible" - ie make it clearer what each test is doing just by reading the code of that test, you can break up your helper method into its obvious parts.

Yes, it will "duplicate" what would otherwise be confined to the single helper as you will now have a number of calls in each test when you could do it with one, but it allows you to give more meaningful names to each helper method.

// Arrange
someHelper = MakeSomeDependency(arg1, arg2, arg3);
sut = MakeSomeObjectUnderTest(someHelper);

// Act
sut.DoSomething();

// Assert
AssertCommonAsserts(sut, expectedvalue1, expectedvalue2);
Assert.AreEqual(sut.SomeProperty, expectedValue);

Note: I am not a fan of multiple asserts in a single test, but there are cases when a single "fact" can only be checked by multiple asserts and I can also think of a number of scenarios in which having some "guard" asserts before the actual "fact" being asserted can help in debugging failures.

If you use a language that supports named parameters, you can make your tests even more intelligible by using that feature so the method call doesn't just convey the values used, but also what those values are used for.

// Arrange
someHelper = MakeSomeDependency(
    knownNames: arg1, 
    unknownNames: arg2, 
    lookingFor: arg3);
sut = MakeSomeObjectUnderTest(someHelper);

// Act
sut.DoSomething();

// Assert
AssertCommonAsserts(
    objectUnderTest: sut, 
    numberOfItemsFound: expectedvalue1, 
    nameOfItemFound: expectedvalue2);
Assert.AreEqual(sut.SomeProperty, expectedValue);

Answer 2

There is nothing wrong with structuring your test code as well as your business code. The point of a test suite is that it expresses clearly how the tested code is supposed to behave, and that it signals a failure if it doesn't. With judicious refactoring and well-chosen names in your test code, you can serve this goal better than with cut-and-paste drudgery.

(However, Matthew is quite right: set-up and tear-down code that is common to all tests belongs into the framework's special methods, not into your custom subroutines.)

Answer 3

You should absolutely factor out common code in unit tests. Messy and badly-structured unit test code just leads to tests which are difficult to understand and difficult to maintain, no matter how beautiful the code they're testing is.

That said, in this particular case if you were using the .NET test framework NUnit this wouldn't be necessary at all - you'd turn the helper into a test method and use the TestCase attribute to get the framework to call it multiple times with different parameter sets. I'd be surprised if there wasn't something equivalent available for Java and other languages.

So do structure unit tests well, but also make sure you've learned the capabilities of the test framework because it might be able to help you.

Answer 4

You should consider using a parameterized Runner using a DataProvider for all valid input for your particular test cases, instead of your upper aproach. I use helper methods only to increase readability of tests, if they have a complex setup in the test and which would clutter the arrange part of the test.

What it could look like is:

@RunWith( DataProviderRunner.class )
public class YourClassTest {

    @Test
    @DataProvider( value = { "value11|value12|value13","value21|value22|value23" }, 
                   trimValues = true, splitBy = "\\|" )
    public void test_something(String value1, String value2, String value3) {
       // action part (dependent on values)

       // assertion part (dependent on values)
    }


    @Before
    public void independendSetup() {
       // set up part (independent of value1, value2, value3)
    }

    @After
    public void independendtearDown() {
       // tear down part (independent of values)
    }
}