3
\$\begingroup\$

As you can see here, I wrote the methods public static int romanToArabic(String number) and public static String arabicToRoman(int number) to convert roman numbers to arabic numbers and vice versa.

I now wrote a unit-testing-class to test these methods. It is my first attempt to write a (real) unit test, so I would like to know your opinion.

Here's the code:

import org.junit.Test;
import static org.junit.Assert.*;

public class TestCases {

    @Test
    public void testRomanToArabic() {
        String[] input = {null, "", "I", "V", "XXXIII", "DCCXLVII", "CMXXIX", "MCCXXXII", "MMMCMXCIX", "MMMMXI", "KMXI", 
                "VX", "VL", "VC", "VD", "VM", "LC", "LD", "LM", "DM", "IL", "IC", "ID", "IM", "XD", "XM", "CXLV", "MIXI", "IXI", "MXIII", "MMMM", "IIII"}; 
        int[] expectedOutput = {-1, -1, 1, 5, 33, 747, 929, 1232, 3999, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 145, -1, -1, 1013, -1, -1};
        for(int i = 0; i < input.length; i++) {
            int k = RomanNumbers.romanToArabic(input[i]);
            try {   
                assertEquals(k, expectedOutput[i]);
            }
            catch(AssertionError assertFailed) {
                System.out.println("Test failed!");
                System.out.println("Input: " + input[i]);
                System.out.println("Output: " + k + "| Expected: " + expectedOutput[i]);
            }
        }
    }

    @Test
    public void testArabicToRoman() {
        int[] input = {-1, 1, 5, 33, 747, 929, 1232, 3999, 4000};
        String[] expectedOutput = {"Error", "I", "V", "XXXIII", "DCCXLVII", "CMXXIX", "MCCXXXII", "MMMCMXCIX", "Error"};
        for(int i = 0; i < input.length; i++) {
            String k = RomanNumbers.arabicToRoman(input[i]);
            try {   
                assertEquals(k, expectedOutput[i]);
            }
            catch(AssertionError assertFailed) {
                System.out.println("Test failed!");
                System.out.println("Input: " + input[i]);
                System.out.println("Output: " + k + "| Expected: " + expectedOutput[i]);
            }
        }
    }
}

Is this the way to go, or is it considered bad practice to put more than one test in a method? Do you have any other suggestions on improving the code?

\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

I have two suggestions:

  1. I like tests to be very easily human-readable. Instead of separate arrays of input and output, which are not aligned with each other, find a way to put the input and the expected output on one line.

The easiest way in this case would be to add a method

private void testRomanToArabic(String input, int expectedOutput) {
    assertEquals(RomanNumbers.romanToArabic(input), expectedOutput);
}

and then use this method repeatedly like

testRomanToArabic("XXXIII", 33);

etc. There are other ways to put input and expected output next to each other (e.g. create an object holding both), but this is the simplest.

  1. Don't catch the AssertionError and print the result yourself. The test should be run by a test runner which will handle this part for you. I always do this from the Eclipse IDE by selecting "Run As JUnit Test", but there is for sure a way to do the same thing from the command line.

By catching the AssertionError, you hide the error from external tools, meaning you have to read the output yourself. If the AssertionError is thrown, your testing framework is aware of the error, which becomes very important when you have hundreds of tests: it can highlight for you the handful that are failing.

\$\endgroup\$
1
\$\begingroup\$

Noise...noise...noise

There's quite a lot of noise in your tests. As has been pointed out by @OpenSauce, part of that is because you're catching the AssertionError. If you stop catching it, then your test runner will report the message embedded in the exception. As it stands, this message will be wrong, because you're passing your parameters to assertEquals the wrong way round:

assertEquals(k, expectedOutput[i]);

They should be:

assertEquals(expectedValue, actualValue)

Another source of noise is the iteration. Having loops like this in your tests obscures the relevant elements of the test. There are two obvious approaches for extracting the iteration aspect from the test.

  1. Extract each iteration into its own test. This allows tests to be given meaningful names and means that when you change related behaviour, you only have to change the impacted test. So, you might end up with some tests like:

    romanToArabic_null_minusOne
    romanToArabic_empty_minusOne
    romanToArabic_I_one
    

    etc. There's some repetition here, but there's a trade off because each individual test is more specific and easier to understand. It also helps you to understand your expectations, does it really make sense for null/"" to return -1, or should it throw an exception?

  2. Use a parameterised test. These allow the test behaviour to be separated from the test data. The test runner will then run the test, for each dataset, displaying the values passed in for each execution. This removes a lot of the need for the custom error handling to log the inputs into the test (which can get forgotten).

You've also got some naming noise in your tests... Rather than calling your TestCases, give it a meaningful name that describes what it's testing. A common practice is to link it in some way to the class being tested, i.e. RomanNumbersTests. Having test at the start of all of your test names also seems redundant. They have @Test attributes and they're in a test class, so the runner can find them. I think of it as the class name, method name (and in the case of parameterised tests the parameters) combining to present a high level overview what each test is doing.

Using meaningful names instead of k, such as calculatedRomanValue or calculatedArabicValue would also make your assertions easier to follow. Anything that makes the tests easier to read is positive.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.