Is my unit test violating "test should do one thing principle"?

Question

I wrote some unit tests around my class and at the end I am confused whether to combine these tests as their reason of failure will be same.

PatternBuilder class

public class PatternBuilder {    
    private static final String PATTERN_INITIALS = "#0.%s";

    String buildPatternFromScale(int scale) {
        return String.format(Locale.ENGLISH, PATTERN_INITIALS, StringUtils.repeat("0", scale));
    }
}

Unit tests for the above implementation

@RunWith(Parameterized.class)
public class PatternBuilderTest {

    private PatternBuilder patternBuilder;

    @Parameterized.Parameter
    public int scale;

    @Parameterized.Parameter(1)
    public String expectedPattern;

    @Before
    public void setUp() {
        patternBuilder = new PatternBuilder();
    }

    @Parameterized.Parameters(name = "buildPatternFromScale({0}) = {1}")
    public static Collection<Object[]> data() {
        return Arrays.asList(new Object[][]{
                {1, "#0.0"},
                {2, "#0.00"},
                {5, "#0.00000"},
        });
    }

    @Test
    public void testShouldVerifyThatNonNullPatternIsBuilt() {
        //given

        //when
        String pattern = patternBuilder.buildPatternFromScale(scale);

        //then
        assertThat(pattern, is(notNullValue()));
    }

    @Test
    public void testShouldVerifyThatCorrectPatternOfSpecifiedScaleShouldBeCreated() {

        //when
        String pattern = patternBuilder.buildPatternFromScale(scale);

        //then
        assertThat(pattern, is(equalTo(expectedPattern)));
    }    
}

Will I violate the 'test should do only one thing' if I combine first test with second such that it asserts for no-null-value and correct pattern string ?

Effectively the one single test will then have following two assertions-

assertThat(pattern, is(notNullValue()));
assertThat(pattern, is(equalTo(expectedPattern)));

Reason - I am thinking to combine them both because if null pattern string is created than it will be fail both tests for one single reason

Answer 1

Given your code: you don't need the first test. The second test makes sure that your builder returns an object that is equal to an expected pattern.

This test will obviously fail if the builder returns a null object!

In that sense: having two different tests doesn't add much value anyway!

In other words: you might start with both test, or more precisely: when you follow TDD; you would probably start with that first test case. Later on, when you have implemented your real builder, and the "real" test case is in place - then you don't need that first check-for-null test case any more.

Keep in mind: any line of source code also resembles cost for you. As long as it is around, it might be read and is required to be understand (to fix bugs or add functionality). Therefore even unit tests should provide "return on investment" to you. Meaning: tests that don't add significant value to your test bucket are subject to deletion.

Edit, based on the changes to the question. The additional test case

@Test
public void testShouldVerifyThatNonNullPatternIsBuilt() {
    //given
    int scale = 1;

is still redundant. And "worse" - keep in mind that @Parameterized tests work somehow different than "normal JUnit" tests. You see, with the @Parameterized runner in place, each test is invoked for each data value. Meaning: the above test is called three times. The idea is that all tests use the data elements. In that sense: it doesn't make sense to have a test in there that does not use the "data values". Especially given the fact that this test here tests something that the other test checks already.

In other words: assertThat(pattern, is(equalTo(expectedPattern))) will fail when pattern is null (unless you happen to put null into your expectedPattern).

Answer 2

I think I'd stick with two separate tests.here. If this was done using TDD, you would most likely end up with two tests anyway.

Although its worth mentioning that a test can have a single concept (rather than a thing), which can mean multiple asserts in a single test case, if all related logically.

As mentioned before they key thing you are looking forward is clarity in the test to quickly identify what fails and why, you way you have it gives you this which is important in terms of the value added from having the test .

Answer 3

One purpose of "One assertion per test method" rule is that only name of the test method will provide you enough information to understand reason why test failed.

Where if you have multiple assertions in test - you need to read failed message to understand why test failed.

Also with multiple assertions, if first assertion fail other will not be executed.
So for getting "full picture" of possible reason you need re-run tests after fixing first assertion

In your particular case assertion for non-null is same as assertion for equality to expected result. Null result will fail always if expected not null.

Answer 4

Will I violate the 'test should do only one thing' if I combine first test with second such that it asserts for no-null-value and correct pattern string ?

In this case I don't think.
Why ? Because the first test method :

assertThat(pattern, is(notNullValue()));

doesn't valid any expected behavior of the buildPatternFromScale() method.

This method is designed to return a Pattern according what it received as parameter.
The single thing to check is that.
By writing a test method to do a not null assertion, you don't cover the specification of the method. You write just a first step to validate it.

If your method could return null in some cases and not null in others, it could make sense.
But it is not the case.

So I would replace :

assertThat(pattern, is(notNullValue()));
assertThat(pattern, is(equalTo(expectedPattern)));

by just :

assertThat(pattern, is(equalTo(expectedPattern)));