Is Assert-ing on tearDown (@After) method wrong?

Question

I have multiple test cases even and if the logic is different, the output must be equal on all of them. So I was thinking in how to generalize them and place the Assert method only once.

Is there any way better to do it than this one:

static public class Tests() {

    private static String expected = null;
    private String actual = null;

    @BeforeClass
    public static void setUpBeforeClass() throws Exception {
        expected = new String("My Desired Output");
    }

    @Before
    public void setUp() {
        actual = new String();
    }

    @Test
    public void test1() throws Exception {
        actual = ...
    }

    @Test
    public void test2() throws Exception {
        actual = ...
    }

    @After
    public void tearDown() throws Exception {
        assertThat(actual, is(equalTo(expected)));
    }

    @AfterClass
    public static void tearDownAfterClass() {
    }
}

Running method:

@Test
public void runTests() {
    Result result = JUnitCore.runClasses(Tests.class);
    assertThat(result.getRunCount(), is(2));
    assertThat(result.getFailureCount(), is(0));
}

Answer 1

If you must generalize then you could create one method like

private void testIt ( String actual )
{ assertThat(actual, is(equalTo(expected))); }

and call it from all your test methods.

If and when a test fails it will be more obvious which test failed.

Answer 2

Yes, asserting in the tearDown method is a bad idea. This method exists, according to the JUnit documentation, to

Tears down the fixture, for example, close a network connection. This method is called after a test is executed.

I think that storing your expected and actual values in the test class are a bad idea in general. These variables are test-dependent, so store them inside your test case and do your assert in the test case. For example:

public class FooTest {

    @Test
    public void testFoo() {
        Object expected = // ...
        Object actual = // ...

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

}

Also, I see in your code that all test have the same expected value. It might be a good idea to vary your tests so returned values are always different. Testing only one expected value all the time make you sure the code works for this expected result. Try with some more, possibly very different, and try to test some corner cases.