Use JUnit’s expected exceptions sparingly

Posted on by lukaseder

Sometimes, when we get pull requests for jOOQ or our other libraries, people change the code in our unit tests to be more “idiomatic JUnit”. In particular, this means that they tend to change this (admittedly not so pretty code):

@Test
public void testValueOfIntInvalid() {
    try {
        ubyte((UByte.MIN_VALUE) - 1);
        fail();
    }
    catch (NumberFormatException e) {}
    try {
        ubyte((UByte.MAX_VALUE) + 1);
        fail();
    }
    catch (NumberFormatException e) {}
}

… into this, “better” and “cleaner” version:

@Test(expected = NumberFormatException.class)
public void testValueOfShortInvalidCase1() {
    ubyte((short) ((UByte.MIN_VALUE) - 1));
}

@Test(expected = NumberFormatException.class)
public void testValueOfShortInvalidCase2() {
    ubyte((short) ((UByte.MAX_VALUE) + 1));
}

What have we gained?

Nothing!

Sure, we already have to use the @Test annotation, so we might as well use its attribute expected right? I claim that this is completely wrong. For two reasons. And when I say “two”, I mean “four”:

1. We’re not really gaining anything in terms of number of lines of code

Compare the semantically interesting bits:

// This:
try {
    ubyte((UByte.MIN_VALUE) - 1);
    fail("Reason for failing");
}
catch (NumberFormatException e) {}

// Vs this:
@Test(expected = NumberFormatException.class)
public void reasonForFailing() {
    ubyte((short) ((UByte.MAX_VALUE) + 1));
}

Give or take whitespace formatting, there are exactly the same amount of essential semantic pieces of information:

  1. The method call on ubyte(), which is under test. This doesn’t change
  2. The message we want to pass to the failure report (in a string vs. in a method name)
  3. The exception type and the fact that it is expected

So, even from a stylistic point of view, this isn’t really a meaningful change.

2. We’ll have to refactor it back anyway

In the annotation-driven approach, all I can do is test for the exception type. I cannot make any assumptions about the exception message for instance, in case I do want to add further tests, later on. Consider this:

// This:
try {
    ubyte((UByte.MIN_VALUE) - 1);
    fail("Reason for failing");
}
catch (NumberFormatException e) {
    assertEquals("some message", e.getMessage());
    assertNull(e.getCause());
    ...
}

3. The single method call is not the unit

The unit test was called testValueOfIntInvalid(). So, the semantic “unit” being tested is that of the UByte type’s valueOf() behaviour in the event of invalid input in general. Not for a single value, such as UByte.MIN_VALUE - 1.

It shouldn’t be split into further smaller units, just because that’s the only way we can shoehorn the @Test annotation into its limited scope of what it can do.

Hear this, TDD folks. I NEVER want to shoehorn my API design or my logic into some weird restrictions imposed by your “backwards” test framework (nothing personal, JUnit). NEVER! “My” API is 100x more important than “your” tests. This includes me not wanting to:

  • Make everything public
  • Make everything non-final
  • Make everything injectable
  • Make everything non-static
  • Use annotations. I hate annotations.

Nope. You’re wrong. Java is already a not-so-sophisticated language, but let me at least use the few features it offers in any way I want.

Don’t impose your design or semantic disfigurement on my code because of testing.

OK. I’m overreacting. I always am, in the presence of annotations. Because…

4. Annotations are always a bad choice for control flow structuring

Time and again, I’m surprised by the amount of abuse of annotations in the Java ecosystem. Annotations are good for three things:

  1. Processable documentation (e.g. @Deprecated)
  2. Custom “modifiers” on methods, members, types, etc. (e.g. @Override)
  3. Aspect oriented programming (e.g. @Transactional)

And beware that @Transactional is the one of the very few really generally useful aspect that ever made it to mainstream (logging hooks being another one, or dependency injection if you absolutely must). In most cases, AOP is a niche technique to solve problems, and you generally don’t want that in ordinary programs.

It is decidedly NOT a good idea to model control flow structures, let alone test behaviour, with annotations

Yes. Java has come a long (slow) way to embrace more sophisticated programming idioms. But if you really get upset with the verbosity of the occasional try { .. } catch { .. } statement in your unit tests, then there’s a solution for you. It’s Java 8.

How to do it better with Java 8

JUnit lambda is in the works:
http://junit.org/junit-lambda.html

And they have added new functional API to the new Assertions class. Everything is based around the Executable functional interface:

@FunctionalInterface
public interface Executable {
    void execute() throws Exception;
}

This executable can now be used to implement code that is asserted to throw (or not to throw) an exception. See the following methods in Assertions

public static void assertThrows(Class<? extends Throwable> expected, Executable executable) {
    expectThrows(expected, executable);
}

public static <T extends Throwable> T expectThrows(Class<T> expectedType, Executable executable) {
    try {
        executable.execute();
    }
    catch (Throwable actualException) {
        if (expectedType.isInstance(actualException)) {
            return (T) actualException;
        }
        else {
            String message = Assertions.format(expectedType.getName(), actualException.getClass().getName(),
                "unexpected exception type thrown;");
            throw new AssertionFailedError(message, actualException);
        }
    }
    throw new AssertionFailedError(
        String.format("Expected %s to be thrown, but nothing was thrown.", expectedType.getName()));
}

That’s it! Now, those of you who object to the verbosity of try { .. } catch { .. } blocks can rewrite this:

try {
    ubyte((UByte.MIN_VALUE) - 1);
    fail("Reason for failing");
}
catch (NumberFormatException e) {}

… into this:

expectThrows(NumberFormatException.class, () -> 
    ubyte((UByte.MIN_VALUE) - 1));

And if I want to do further checks on my exception, I can do so:

Exception e = expectThrows(NumberFormatException.class, () -> 
    ubyte((UByte.MIN_VALUE) - 1));
assertEquals("abc", e.getMessage());
...

Great work, JUnit lambda team!

Functional programming beats annotations every time

Annotations were abused for a lot of logic, mostly in the JavaEE and Spring environments, which were all too eager to move XML configuration back into Java code. This has gone the wrong way, and the example provided here clearly shows that there is almost always a better way to write out control flow logic explicitly both using object orientation or functional programming, than by using annotations.

In the case of @Test(expected = ...), I conclude:

Rest in peace, expected

(it is no longer part of the JUnit 5 @Test annotation, anyway)

Published by lukaseder

I made jOOQ

33 thoughts on “Use JUnit’s expected exceptions sparingly”

        1. It suffers from the same flaw (although that’s clearly a matter of taste). It scopes the expectation to the whole method, rather than a single statement…

          Reply

          1. No, it does not! Scoping is different.

            If the expected exception is set just before the call to the method to test one can be sure that no exception during the test setup will be enough for the test to pass, e.g.

            when(mockA.foo()).thenReturn(bar);
when(mockB.bar()).thenReturn(foo);
expectedException.expect(RuntimeException.class) // before the rule was set to ExpectedException.none(), so bugs in when(...) wouldn't suffice.
result = service.doIt(); // only now RuntimeExceptions will trigger the rule
// assertThat(result, is(true)); // pointless, must not get here ;-)

            In contrast to something like @Test(expected=RuntimeException.class) that would be green for bogus test setups, too.

            Reply

  2. Hi, Lukas — me again. :-)

    I agree with 99.5% of your conclusions.

    But when you say “My API is 100x more important than ‘your’ tests, be careful. If, for example (my usual one-trick pony here), your API makes everything so ‘final’ that I can’t mock part of it out in my tests… then I’m simply not going to use your API. Period. IOW, if you want people to use your API, you have to allow for the fact that we need to do *some* kinds of reasonable testing.

    Reply

    1. So, we have a deadlock and no one wins. Too bad!

      Stay tuned. That article about the anti-final cargo cult is overdue. It’ll be published very soon :)

      Reply

      1. Just remember that absolutes are the province of religion, not engineering. :-)

        In my experience, it’s rare to find a case that doesn’t have a reasonable compromise about the truly important parts. The rest is “religion” in the pejorative sense. I look forward to your article.

        Reply

  3. Hey, I really can’t agree with most of your statements here.

    1.

    “We’re not really gaining anything in terms of number of lines of code”

    // This:
try {
    ubyte((UByte.MIN_VALUE) - 1);
    fail("Reason for failing");
}
catch (NumberFormatException e) {}
 
// Vs this:
@Test(expected = NumberFormatException.class)
public void reasonForFailing() {
    ubyte((short) ((UByte.MAX_VALUE) + 1));
}

    This really isn’t a fair comparison. The whole method, on one hand, versus only a part of it?

    If you really want to do a fair “number of lines of code” comparison, then you should really do the comparison this way:

    // This:
@Test
public void reasonForFailing() {
  try {
    ubyte((UByte.MIN_VALUE) - 1);
    fail("Reason for failing");
  }
  catch (NumberFormatException e) {}
}
 
// Vs this:
@Test(expected = NumberFormatException.class)
public void reasonForFailing() {
  ubyte((short) ((UByte.MAX_VALUE) + 1));
}

    Clearly there really is a difference in number of lines of code. It is boiler-plate code that was removed. Code that does not provide any additional information and only makes the important pieces less apparent.

    2.

    “The single method call is not the unit. The semantic unit being tested is that of the behaviour in the event of invalid input in general.”

    That is, in my opinion, completely incorrect. The name of the method is poorly chosen, too.

    testValueOfIntInvalid

    A method name should describe what the method does. As closely, as possible. For test methods, this is equally important! The name of a test method should tell me what’s being tested. Your method name doesn’t really tell me much. You’re testing the behaviour in case of invalid input. But what exactly is invalid input? That’s what I want to know when using your unit within my own code, and that’s what your tests should tell me.

    A method should never have more than one well defined, coherent responsibility (i.e. do more than one thing). And a single test method should really not test more then one thing. If you have a unit for which invalid input can have say 10 different forms, will you really test all those 10 cases in a single test method?

    So no, this is not a good test method name:

    testValueOfIntInvalid

    And no, this is really not much better:

    testValueOfShortInvalidCase1()
    testValueOfShortInvalidCase2()

    Instead, you should name those two methods like this, for example:

    doesNotAcceptValuesGreaterThanMax()
    doesNotAcceptValuesLesserThanMin()

    3.

    “I NEVER want to shoehorn my API design or my logic into some weird restrictions imposed by your “backwards” test framework! My API is 100x more important than your tests.”

    The fact that code is easy to test is not the main goal. It is a by-product. Code maintainability correlates with testability. In other words, if your code is hard to test, it is probably also hard to reason about, hard to maintain.

    4.

    “Annotations are always a bad choice for control flow structuring.”

    I read that section multiple times. I still couldn’t find any arguments to support your statements. I really don’t see a reason why an annotation is a bad choice in this case.

    Overall, the only think on which I agree with you is the fact that this way you can only test the class of your exception and not it’s additional parameters, such as the message.

    Reply

  4. JUnit 4 always felt like a poor copy of TestNG, so it should be surprising that some aspects were only half implemented. In TestNG there is expectedExceptionsMessageRegExp for your scenario. That’s not to say it was ever promoted as the “right” approach, which is always to do whatever is the clearest and least error prone.

    Custom annotations on test methods is extremely powerful and useful for complex APIs. For parameterized tests, they can be used to define a specification to generate only the parameter combinations are compatible. You can then automatically validate the parameters (e.g. internal state of a data structure) if the test passes, e.g. to catch corruption. By using other custom annotations you might opt into more validation to detect improper calls.

    This leads to short tests that focus on the behavior and expected outcome, and all the shared boilerplate can be abstracted away. That makes tests easier to debug, easier to opt into more variations, and provides a higher confidence when refactoring.

    So when annotations are used correctly it can really help scale tests with the project. But, someone will always find a way to abuse any tool you give them.

    Reply

    1. “expectedExceptionsMessageRegExp” – that sounds like an awful specific method :)

      Sure you can eventually build anything on top of annotations. But you can, too, using explicit programming. Including parameter generation. It’s just that annotations are so awfully rigid, they cannot be extended or recombined in unforeseen ways…

      Reply

  5. do you think Javaslang may help in such cases? you know by using the static Try for instance

    so much love for such statement:
    “functional programming beats annotations every time”

    Reply

    1. Huh, it might indeed – I haven’t thought about that. Personally, I think I’ll find the assertThrows() approach more concise. I’m asserting that a given lambda throws a given exception…

      Reply

    1. I don’t know. I just really don’t like those testing DSLs. I mean, why does a test need to read like pseudo English, while all my API programming skills now evolve around concise functional programming? I would never use a DSL to express business logic, like

      whenCustomerOrder(order).greaterThan(9)
                        .andHasMessageContaining("asdf")
                        .then(send(freegoodie()));

      I mean, what’s wrong with classic Java?

      if (order.abc() > 9 && order.message().contains("asdf"))
    send(...);

      When did that fad of putting testing DSLs everywhere even start?

      Anyway, this is deeply subjective, so there will be no clear answers :)

      Reply

  7. What’s your opinion on using Junit rules for testing excepton handling when Java 8 is not an option?

    Reply

      1. Agreed. The only problem with try-catch is its verbosity, which is something I can live with (IMHO a lone catch would do better, the implicit try would apply to all preceding code in the block; just syntactic sugar).

        I’ve always considered JUnit 4 to be a step in the wrong direction. It has some improvements, but they don’t even give enough to compensate for the time lost by a forgotten @Test annotation. While JUnit 3 has a single hack (test method name magic), JUnit 4 has many of them (they’re all documented, but otherwise non-discoverable; overriding setUp is much more obvious than using @BeforeWhatever).

        Reply

        1. IMHO a lone catch would do better

          You mean like this?

          {
    something();
}
catch (Exception e) { ... }

          Hmm, indeed, the try is a bit superfluous. I have never thought about it this way. In PL/SQL, for instance, the “catch” block (i.e. EXCEPTION) is always available on any anonymous block, e.g.

          -- No "catch" block
BEGIN
  NULL;
END;

-- With "catch" block
BEGIN
  NULL;
EXCEPTION
  WHEN OTHERS THEN NULL;
END;

          So, no special “try” keyword needed. I think I will have to blog about this! :) Will give you credit, obviously.

          Interesting criticism about JUnit4. I have never thought about these things, even if I’m usually not much in favour of using annotations. But somehow, in these testing framework, anything goes, so I never bothered…

          Reply

          1. You mean like this? …

            No, you’d save just the keyword. I’d like to save the braces and the indentation (think version control). I mean

            something1();
something2();
catch (Exception e) { ... }

            protecting everything above in the catch clause. This covers most use cases and if you needed more, then use blocks:

            unprotected1();
{
    protected1();
    protected2();
    catch (Exception e) { ... }
}
unprotected2();

            I guess, that’s exactly PL/SQL.

            I dislike both JUnit 3 and 4. And I guess, I won’t like 5 either. For JUnit 3, something like 

            abstract class AbstractTest {
    protected void beforeClass() {}
    protected void afterClass() {}
    protected void beforeMethod() {}
    protected void afterMethod() {}
}

            would be the obvious choice, avoiding questions like this and discussions like “when does setup get executed?” (or was it “setUp”?).

            There might be cases when your test method can’t inherit from a common abstract class (ever seen such a case?), so the next version should add equivalent annotations.

            Actually, I feel like JUnit has done about everything wrong. To keep it short, I give just a list of things I’d add

            enum ExecutionOrder {ORDERED, RANDOMIZED, CONCURRENT}
            protected boolean newInstancePerTestMethod() {return false;}
            class TestSkippedException extends RuntimeException {….}

            Reply

            1. You’re right, it occurred to me later on while writing the post. In fact, you’re suggesting the same thing as I did, except that your suggestion is more thorough. You don’t distinguish between block statements and “single statement” statements. So, your suggestion makes total sense (and would be very useful). We could take this one step further and make the curly braces after catch (or after finally) optional by removing them from the specs and expecting just an ordinary statement after catch or finally. E.g.

              something2();
  catch (SQLException e)
    log.info(e);
  catch (IOException e)
    log.warn(e);
  finally
    close();

              Very nice!

              Actually, I feel like JUnit has done about everything wrong

              Well, the story goes, it was hacked together by two gang-of-four members during a cross Atlantic flight. You cannot expect too much from that :)

              Reply

        2. Fascinating, from a psychological perspective. To me, having the behavior of the test class depend on the *names* of the methods is horrifying, and the @Test, @Before, etc. annotations in Junit 4 were the beginnings of sanity. A sort of metaphorical SRP: the name of the method is the name of the method, and not a ‘switch’ that chooses whether the method is executed (or not).

          Reply

            1. (Hmm, this is a reply to Lukas’ “not a big fan… query by method name feature” comment.)

              Ding! Yep, I hate it. A name is a name is a name. “The symbol is not the thing: the map is not the territory.”

              Refactoring tools should IMHO be able to rename methods w/o breaking anything. If we mix names and actions, we might as well just declare java variables by name instead of type, let’s say, oh, “var StringMyName” and be done with it.

              I have (previously) called this Roth’s Corollary to Clarke’s 3rd law: “Any sufficiently confusing magic is NOT necessarily an advanced technology”. Using method names to cause actions is LITERALLY the definition of “sympathetic magic”.

              See also Phlebotinum (courtesy of Buffy the Vampire Slayer) and Handwavium, charming articles at http://tvtropes.org/pmwiki/pmwiki.php/Main/ClarkesThirdLaw and http://tvtropes.org/pmwiki/pmwiki.php/Main/AppliedPhlebotinum

              Reply

          2. (in reply to Charles Roth)

            AFAIK the only strange thing was depending on test method name starting with “test”. Strange, but much more robust than depending on something as easy to forget as @Test. If I were to introduce such an annotation, I’d add @NoTest too, and I’d require every public non-static no-args method to get one of them.

            All the other features (setUp, etc.) were just overriding methods. No, you can’t rename them, but that’s how overriding works, isn’t it?

            Reply

          1. Hmm, having trouble replying to the right comment… this is in reply to Maaartinus, above.

            Fair point, although Junit 3 was all about extending a base class. That’s gone from Junit 4, so now we can name our @Before method whatever we want.

            I would not be averse to the @NoTest annotation idea! I can see value in it.

            Wait, I see it! We annotate the class with @MustHaveAnnotations, then each method MUST have @Test or @NoTest, and … (ducks and covers before Lukas can slap him silly).

            Reply

  8. I agree. For JUnit, I find the try {something(); fail();} catch (SomeException e){} pattern to be the most flexible and the one that lets me test the precise error conditions.

    Reply

  9. When there are thousands of unit tests to maintain (often by someone who has never seen the code), keeping test code as short and as easy to understand as possible makes maintnance easier. That said, I prefer to write a JUnit rule to validate error conditions. This saves countless lines of test code and can be reused by many projects in an Enterprise.

    Reply

    1. It depends :) Short != Readable. OK, the expect annotation is obvious, but if the test developer had to write tons of extra methods just to make use of it…?

      Reply

Leave a Reply to JonCancel reply