4

I am writing a Java Unit test for one of my method. The method declaration is like this:

public int convertToInteger() throws InvalidRomanNumberException
{
    int result=0;
    BaseRomanNumeral num1, num2; 
    int i=0;
    if(!validOperation())
        throw new InvalidRomanNumberException();
}

Now I am trying to write two unit tests. One is to test if the right exception is thrown. Another one is to make sure that that the write conversion happens. This is how my test case looks

@Test
public void testRomanNumberConversion() {
    String romanValue="MCMII";
    RomanNumber num=new RomanNumber(romanValue);
    assertEquals(1903,num.convertToInteger());  
}

@Test(expected = InvalidRomanNumberException.class)
public void testInvalidRomanNumberExceptionThrown()  {
    String romanValue="MCMIIII";
    RomanNumber num=new RomanNumber(romanValue);
    num.convertToInteger(); 
}

For both these test cases I am getting an error saying Unhandled InvalidRomanNumberException. This is resolved only when I add throws InvalidRomanNumberException to each method definition. But I don't think that is the right way. Just want to check with the rest of you, what is the norm here? How should I resolve this unhandled exception message

Chetan Kinger
  • 15,069
  • 6
  • 45
  • 82
gazubi
  • 561
  • 8
  • 32

2 Answers2

5

Since it looks like InvalidRomanNumberException is a checked exception, you have to either surround it with a try-catch or declare that the method throws InvalidRomanNumberException. JUnit or not, this is the norm.

That being said, the test case method that you expect will throw a InvalidRomanNumberException should ideally declare that it throws one since there is no point suppressing it with a try-catch as your test case will fail. On the other hand, the test case method that you expect will not throw an exception can use a try-catch around the convertToInteger method and regardless of whether an exception is thrown, this test case should have an assert on the expected result from convertToInteger method.

The end result of a JUnit test case should be whether the test passed or failed. An exception at runtime would indicate neither. A JUnit test case must not crash.

Chetan Kinger
  • 15,069
  • 6
  • 45
  • 82
  • so your suggestion is I just add a throws InvalidRomanNumberException right. And if there is an exception then the test will just fail. So there is no need to add try catch anywhere else in the program for this testcase method that is throwing an exception – gazubi May 01 '15 at 08:32
  • @bob_d : No. My suggestion is that the method where you don't expect an exception should use a `try-catch` and the one where you do expect one should use `throws`. In the former case, you should be asserting the result that you expect from the `convertToInteger` method regardless of whether an exception is thrown or not. In the later, you are asserting that the exception is `expected`. See my edit of the last few lines in the second paragraph. – Chetan Kinger May 01 '15 at 08:35
  • Can the downvoter please leave a comment? Not sure why an accepted answer would warrant a down vote – Chetan Kinger May 02 '15 at 05:00
  • There's no reason to add a `try` ... `catch` block in `testRomanNumberConversion()`. The simplest think that can work is just to add `throws InvalidRomanNumberException`. It does no harm to add `throws InvalidRomanNumberException` if the test case will never cause the exception to be thrown. If the test throws that exception--or any exception--the test will fail, because JUnit itself will catch the exception and report the test as failed (providing a useful stack trace of the failure!). Tests should be made as simple as possible (but no simpler). – NamshubWriter May 02 '15 at 10:17
  • So you downvoted this because you disagree with my opinion? Do you think leaving a comment instead of downvoting would have been a better option? Please read this : http://stackoverflow.com/help/privileges/vote-down. Is my answer sloppy, clearly or dangerously incorrect? – Chetan Kinger May 02 '15 at 10:29
  • I downvoted because it is bad advice and violates JUnit conventions. Adding a try catch to avoid a throws clause hides a useful exception from the JUnit output and makes debugging test failures more difficult. I think that qualifies. If others disagree they can upvote your answer – NamshubWriter May 02 '15 at 15:42
  • So the right thing to do in this case would be to declare that all the tests that test `covertToInteger` should declare that they throw `InvalidRomanNumberException`? – Chetan Kinger May 02 '15 at 16:20
  • The test methods should either declare that they throw InvalidRomanNumberException or they should simplify declare that they throw Exception. Assuming, of course, that the exception shouldn't be a runtime exception. – NamshubWriter May 02 '15 at 16:50
  • @NamshubWriter That makes sense. I was not aware of such a convention until you mentioned it so the downvote is justified. I answered this question from what I thought was the most logical way of going about it (and the way I have been doing it so far). Looks like I have not been following the general convention but I wouldn't change the way I have been going about it. I still prefer that the tests that need to fail on a value should fail on a value rather than an exception. – Chetan Kinger May 02 '15 at 17:01
4

This feels more like it should be an unchecked exception as opposed to a checked exception.

Recall the difference between the two: a checked exception is meant to be something that's reasonably recoverable from, like a missing file or a malformed URL. An unchecked/run time exception is meant to be something that is irrecoverable, like dividing by zero.

If a user enters in an invalid Roman numeral, it might not make sense to say that they can recover and try again - the conversion layer shouldn't be responsible for that. It sounds more like a thing should be decided at instantiation time.

If you instead make your custom exception extend RuntimeException, then you won't need to declare it to be thrown (and if you did, it wouldn't have any effect), and you won't have to deal with it in your tests.

The alternative would be to declare it to be thrown in your tests instead. This has the advantage of allowing you to keep these exceptions as checked and ensuring that the tests won't complain about you not handling the potential exception from being uncaught or unthrown.

@Test
public void testRomanNumberConversion() throws InvalidRomanNumberException {
    String romanValue = "MCMII";
    RomanNumber num = new RomanNumber(romanValue);
    assertEquals(1903, num.convertToInteger());  
}

@Test(expected = InvalidRomanNumberException.class)
public void testInvalidRomanNumberExceptionThrown() throws InvalidRomanNumberException {
    String romanValue = "MCMIIII";
    RomanNumber num = new RomanNumber(romanValue);
    num.convertToInteger(); 
}
Makoto
  • 104,088
  • 27
  • 192
  • 230
  • I don't completely agree with the suggestion of declaring `throws` for all test case methods. A test that is not expecting an exception should not result in programming exception at runtime because of an exception. The JUnit test case should not crash. It should be asserting the final result of the `convertToInteger` method, suppressing any exceptions. The end result of such a test case should be whether the test passed or failed. An exception at runtime would indicate neither. – Chetan Kinger May 01 '15 at 08:41
  • Hey thanks for your detailed explanation. It really helps me understand checked and unchecked exceptions better – gazubi May 01 '15 at 08:46
  • 1
    @ChetanKinger: I don't necessarily disagree with your sentiment, hence why I suggested changing it to an unchecked exception *first*. My belief is that exceptions that are actually recoverable do need to be checked, but in this scenario, I feel like this sort of declaration is fine. The only way that the above exception would be thrown is if an invalid Roman numeral is entered, and at least with the first test case, it will not occur. – Makoto May 01 '15 at 09:01