Advantage/Disadvantage of having all variables declared in a JUnit Test

Question

I've been writing some unit tests for some new code at work, and sent it off for a code review. One of my co-workers made a comment about why I was putting variables that are used in a number of those tests outside of the scope for the test.

The code I posted was essentially

import org.junit.Test;

public class FooUnitTests {

    @Test
    public void testConstructorWithValidName() {
        new Foo(VALID_NAME);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithNullName() {
        new Foo(null);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithZeroLengthName() {
        new Foo("");
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithLeadingAndTrailingWhitespaceInName() {
        final String name = " " + VALID_NAME + " ";
        final Foo foo = new Foo(name);

        assertThat(foo.getName(), is(equalTo(VALID_NAME)));
    }

    private static final String VALID_NAME = "name";
}

His proposed changes were essentially

import org.junit.Test;

public class FooUnitTests {

    @Test
    public void testConstructorWithValidName() {
        final String name = "name";
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithNullName() {
        final String name = null;
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithZeroLengthName() {
        final String name = "";
        final Foo foo = new Foo(name);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorWithLeadingAndTrailingWhitespaceInName() {
        final String name = " name ";
        final Foo foo = new Foo(name);

        final String actual = foo.getName();
        final String expected = "name";

        assertThat(actual, is(equalTo(expected)));
    }
}

Where everything that is required within the scope of the test, is defined within the scope of the test.

Some of the advantages he argued were

1. Each test is self contained.
2. Each test can be executed in isolation, or in aggregation, with the same result.
3. Reviewer doesn't have to scroll to wherever these parameters are declared to look up what the value is.

Some of the disadvantages of his method that I argued were

1. Increases code duplication
2. Can add noise to the reviewers mind if there are several similar tests with different values defined (ie. test with doFoo(bar) results in one value, while the same call has a different result because bar is defined differently in that method).

Aside from convention, are there any other advantages/disadvantages of using either method over the other?

Answer 1

You should keep doing what you're doing.

Repeating yourself is just as bad an idea in test code as in business code, and for the same reason. Your colleague has been misled by the idea that every test should be self-contained. That's quite true, but "self-contained" doesn't mean that it should contain everything it needs within its method body. It only means that it should give the same result whether it's executed in isolaton or as part of a suite, and regardless of the order of the tests in the suite. In other words, the test code should have the same semantics regardless what other code has executed before it; it doesn't have to have all required code bundled textually.

Reusing constants, set-up code and the like increases the quality of the test code and doesn't compromise its self-contained-ness, so it's good thing that you should keep doing.

Answer 2

It depends. In general, having repeating constants declared in only one place is a good thing.

However, in your example, there is no point in defining a member VALID_NAME in the shown way. Assumed in the second variant a maintainer changes the text name in the first test, the second test will most probably be still valid, and vice versa. For example, lets assume you want additionally test upper and lower case letters, but also keep a test case just with lower case letters, with as few test cases as possible. Instead of adding a new test, you could change the first test to

public void testConstructorWithValidName() {
    final String name = "Name";
    final Foo foo = new Foo(name);
}

and still keep the remaining tests.

In fact, having a lot of tests depending on such a variable, and changing the value of VALID_NAME later may unintentionally break some of your tests at a later point in time. And in such a situation, you can indeed improve the self-containment of your tests by not introducing an artificial constant with different tests depending on it.

However, what @KilianFoth wrote about not repeating yourself is correct, too: follow the DRY principle in test code the same way you do it in business code. For example, introduce constants or member variables when it is essential for your test code that their value is the same in every place.

Your example show how tests tend to repeat initialization code, that's a typical place to start with refactoring. So you may consider to refactor the repetition of

  new Foo(...);

into a separate function

 public Foo ConstructFoo(String name) 
 {
     return new Foo(name);
 }

since it will allow you to change the signature of the Foo constructor at a later point in time more easily (because there are fewer places where new Foo is actually called).

Answer 3

I would actually... not go for both, but I'll pick yours over your co-worker for your simplified example.

First things first, I tend to favor inlining values instead of doing one-off assignments. This improves code readability for me as I do not have to break my train of thought into multiple assignment statements, then read the line(s) of code where they are used. Therefore, I will prefer your approach over your co-worker, with the slight nitpick that testConstructorWithLeadingAndTrailingWhitespaceInName() can probably be just:

assertThat(new Foo(" " + VALID_NAME + " ").getName(), is(equalTo(VALID_NAME)));

That brings me to the naming. Usually, if your test is asserting an Exception to be thrown, your test name should describe that behavior too for clarity. Using the example above, so what if I have extra whitespaces in the name when I construct my Foo object? And how is that related to throwing an IllegalArgumentException?

Based on the null and "" tests, I am presuming here that the same Exception is thrown this time for calling getName(). If that's indeed the case, maybe setting the test method's name as callingGetNameWithWhitespacePaddingThrows() (IllegalArgumentException - which I think will be part of JUnit's output, hence optional) will be better. If having whitespace padding in the name during instantiation will also throw the same Exception, then I'll skip the assertion entirely too, to make it clear that it's in the constructor's behavior.

However, I think there's a better way of doing things here when you get to more permutations for valid and invalid inputs, namely parameterized testing. What you are testing is exactly that: You are creating a bunch of Foo objects with different constructor arguments, and then asserting it either fails at the instantiation or calling getName(). Therefore, why not let JUnit perform the iteration and scaffolding for you? You can, in layman terms, tell JUnit, "these are the values I want to create a Foo object with", and then have your test method assert for IllegalArgumentException in the right places. Unfortunately, since JUnit's way of parameterized testing doesn't work well with testing for both successful and exceptional cases in the same test (as far as I'm aware of), you'll probably need to jump through a bit of a hoop with the following structure:

@RunWith(Parameterized.class)
public class FooUnitTests {

    private static final String VALID_NAME = "ABC";

    @Parameters
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][] {
                 { VALID_NAME, false }, 
                 { "", true }, 
                 { null, true }, 
                 { " " + VALID_NAME + " ", true }
           });
    }

    private String input;

    private boolean throwException;

    public FooUnitTests(String input, boolean throwException) {
        this.input = input;
        this.throwException = throwException;
    }

    @Test
    public void test() {
        try {
            Foo test = new Foo(input);
            // TODO any testing required for VALID_NAME?
            if (throwException) {
                assertEquals(VALID_NAME, test.getName());
            }
        } catch (IllegalArgumentException e) {
            if (!throwException) {
                throw new RuntimeException(e);
            }
        }
    }
}

Admittedly, this looks clunky for what is an otherwise simple four-value testing, and it isn't helped much by JUnit's somewhat restrictive implementation. In this regard, I find TestNG's @DataProvider approach to be more useful, and it's definitely worth a closer look if you are considering parameterized tests.

Here's how one might be able to use TestNG for your unit tests (using Java 8's Stream to simplify the Iterator construction). Some of the benefits are, but not limited to:

1. Test parameters become method arguments, not class fields.
2. You can have multiple @DataProviders providing different kinds of inputs.
   • It is arguably clearer and easier to add new valid/invalid inputs for testing.
3. It still looks a bit overblown for your simplified example, but IMHO it's neater than JUnit's approach.

public class FooUnitTests {

    private static final String VALID_NAME = "ABC";

    @DataProvider(name="successes")
    public static Iterator<Object[]> getSuccessCases() {
        return Stream.of(VALID_NAME).map(v -> new Object[]{ v }).iterator();
    }

    @DataProvider(name="exceptions")
    public static Iterator<Object[]> getExceptionCases() {
        return Stream.of("", null, " " + VALID_NAME + " ")
                    .map(v -> new Object[]{ v }).iterator();
    }

    @Test(dataProvider="successes")
    public void testSuccesses(String input) {
        new Foo(input);
        // TODO any further testing required?
    }

    @Test(dataProvider="exceptions", expectedExceptions=IllegalArgumentException.class)
    public void testExceptions(String input) {
        Foo test = new Foo(input);
        if (input.contains(VALID_NAME)) {
            // TestNG favors assert(actual, expected).
            assertEquals(test.getName(), VALID_NAME);
        }
    }
}