Testing the wording of an Exception message

Question

Doing a code review, I ran into this assertion in a unit test:

assertThatThrownBy(() -> shoppingCartService.payForCart(command))
  .isInstanceOfSatisfying(PaymentException.class, 
    exception -> assertThat(exception.getMessage()).isEqualTo(
      "Cannot pay for ids [" + item.getId() +"], status is not WAITING"));

I feel that testing the literal text of an Exception is bad practice, but I'm unable to convince the author. (Note: this is not localized text; it's a String literal in the code with the item ID as a parameter.) My reasoning:

• If we decided that, e.g. the item's ID was a critical piece of information, it might make sense to test that the message contains the ID. But in this case, it would be better to include the ID as a field of the Exception class.
• If we had some sort of external system that was automatically reading the logs and looking for certain strings (we don't), then yes, this test would be justified
• What's important in the message is that it clearly communicates what the problem is. But there must be hundreds of ways of constructing such a message, and no testing library can tell us if a plain text human language String is "clear" or "useful".
• Thus, this amounts to, e.g. writing unit tests for translations - it's pointless because the best you can do boils down to duplicating your messages file in the tests.

What's the best practice here, and why?

Answer 1

I feel that testing the literal text of an Exception is bad practice

Never try to interpret the wording of an Exception message. They're just too darned easy for somebody [else] to change!

If the handlers of an specific Exception need specific data, then these should be added as custom properties on a custom Exception class (which the handlers can specifically look to catch). The handling code can then obtain the information it requires unambiguously and reliably.

OK, if the message happens to contain a textual representation of the same information, that's fine, but application code must not rely on it.

it would be better to include the ID as a field of the Exception class

Absolutely, yes.

If we had some sort of external system that was automatically reading the logs

If you were putting data into Exception messages that were being examined by a.n.other system or made available to a.n.other group of users, then you should be [very] careful about exactly what data you're sending - "logging" somebody's name and date-of-birth in the same Exception message might get you into [very] hot water, these days! (GDPR)

What's important in the message is that it clearly communicates what the problem is

Only if that message is going to wind up in front of a User.

To me, Structured Exception Handling is all about the Handling - doing something "useful" with each Exception. In this context, having the application throw it up in front of the User and "give up" is the very loosest definition of "useful"; a last resort.

Ideally, you want [code] to handle the Exception and make it "disappear" before the User is even aware of it.

Answer 2

The main point of testing the exception message content is to make sure the right exception is thrown.

There may be multiple reasons for which payForCart() throws a PaymentException.

So it's not about the exact wording, but about whether it says "Cannot pay for ids A9dr6L, status is not WAITING" and not "Cannot pay for ids A9dr6L, balance insufficient". The alternative would be to have a different exception class for every single throw statement.

Answer 3

I see your point and I also try to avoid comparing exception messages. As Michael Borgwardt already mentioned, a dedicated exception type may help.

Adding exception types for every kind of exception is however not practical, so it depends on the number of exception types you want to explicitly check and how that fits into your overall design.

Personally, I would settle for a compromise and not check on equality of the message text but instead check if the message contains certain keywords, or phrases, e.g. "status is not WAITING."

As for best practice, I would recommend enhancing the exception type PaymentException with the required information. Add a property Reason which is for example of an enumeration type with the possible values (InvalidStatus, InsufficientFunds, etc.). This way, the exception message can be localized and changed without impact on the error handling code, the number of exception types remains manageable and an eventual mismatch between throwing and handling code is detected at compile time.

Answer 4

There are situations where the message isn't important to the code, but is important to a troubleshooting programmer. You may not ever want to add code to parse the IDs out of the message, but the first thing a programmer is going to want to know when troubleshooting that failure is which IDs failed.

In other words, having certain information in the error message is sometimes part of the requirements. If the error message is required, then it should be tested, at least to the same levels you test other kinds of output. How many times has troubleshooting been delayed because the logs said something useless like "Cannot pay for IDs ItemArray@0xdeadbeef"?

Answer 5

In Test-Driven Development by Example, Kent Beck introduced "two simple rules". The first of those rules

Write a failing automated test before you write any code

(Emphasis added.)

The construction of the exception, and its message, certainly counts as code, so if you accept that this rule applies, there should definitely be a matching test.

Michael Feathers, in Working Effectively With Legacy Code, writes that a good unit test has these qualities

1. They run fast
2. They help us localize problems

The test you describe here isn't obviously a problem for speed (it's not obviously more expensive than payForCart) -- we're really just looking at different in memory representations of the same information.

"Help us localize problems" is kind of interesting. There are actually a lot of different decisions being evaluated here, which might indicate that this design needs more modules (see Parnas 1971).

In other words, we could have a test that checks that the correct exception is thrown

assertThatThrownBy(() -> shoppingCartService.payForCart(command))
    .isEqualTo(
        PaymentException.forItem(
            item
        )
    );

And then elsewhere write other tests to ensure that PaymentException::forId has the right behavior.

Of course, carving things apart this way means more investment, both in the tests and in the design. And you do want to ensure that effort invested is balanced by business value returned.

What's important in the message is that it clearly communicates what the problem is. But there must be hundreds of ways of constructing such a message, and no testing library can tell us if a plain text human language String is "clear" or "useful"

Ah - you may want to do some reading of Michael Bolton and James Back on Testing vs Checking. The automated thing we have here is a check; it's an automated confirmation that the behavior today matches the behavior documented by the programmer.

The test is when the human being (be it the programmer, or the reviewer) looks at the message to decide whether it is clear or useful.

If that's what you want to measure against, we'd probably write the test somewhat differently

assertThatThrownBy(() -> shoppingCartService.payForCart(command))
    .isInstanceOfSatisfying(
        PaymentException.class,
        exception -> 
            assertThat(
                 exception.getMessage()
            )
            .isEqualTo(
                "Cannot pay for ids [12345], status is not WAITING"
            )
    );

In this case, we deliberately add duplication to the test to make the intended behavior more explicit, and therefore easier to evaluate in review.

Another way of expressing the same idea; human beings are flexible pattern matching machines, but machines aren't. Therefore, we need to be designing our interfaces with those two different constraints in mind. That suggests a somewhat different test design (since tests are not flexible pattern matching machines).

assertThatThrownBy(() -> shoppingCartService.payForCart(command))
    .isInstanceOfSatisfying(
        PaymentException.class,
        exception -> 
            assertThat(
                 PaymentException.cast(exception).ids()
            )
            .isEqualTo(
                List.of(12345)
            )
    );

And then implement the check of the human readable message elsewhere.

I don't particularly consider "duplicating your messages file in the tests" to be a down side, in so far as the messages file itself is an implementation details that we might want to change without breaking the observable behavior. That's especially so when the messages file is going to be a bunch of templates for the machines to use, meaning that we are sacrificing some of the human readability there.

In practice, I would let this example slide by without comment during a review. Yes, it could be better, but as is it is unlikely to cause harm

• It's test code, therefore it isn't going to impact production at all
• The test is fast and isolated, so it won't be adding additional drag to development processes
• We're probably not changing the behaviors of our exception messaging very often, so over fitting a particular behavior is not likely to add drag.

In short, while the test as implemented doesn't achieve the Platonic ideal, it is Mostly Harmless, and our attention is better invested elsewhere.

Answer 6

I would argue that this crucial information shouldn't simply be stored as a String in the exception. The exception should hold this information as a field, so that any code catching it can access it without parsing the message string.

public class PaymentException extends RuntimeException {
    private int itemID;
    private Reason reason;

    public static enum Reason {
        INCORRECT_STATUS, NOT_ENOUGH_FUNDS, BAD_PAYMENT_METHOD, ITEM_NOT_FOUND;
    }

    public PaymentException(int itemID, Reason reason) {
        super("Error processing payment for item #"+itemID+": "+reason);
        this.itemID = itemID;
        this.reason = reason;
    }

    public int getItemID() {
        return itemID;
    }

    public Reason getReason() {
        return reason;
    }
}

Answer 7

tl;dr– Instead of comparing exception messages, the code should generate a duplicate-exception from the same constructor, then call the duplicate-exception to check for consistency.

---

Testing the exception messages looks flimsy.

Instead:

1. Define a public class for exceptions.

2. When a thrower wants to throw, it must use the standard constructor to generate the exception to be thrown.

3. When a catcher wants to check, it must:

   1. Use the same standard constructor to generate a duplicate-exception.

   2. Use a standard equality/consistency-checking method to compare the caught-exception to the duplicate-exception.

---

This is basically the same thing that the code's already doing, except:

1. Abstracting the exception implementation details.

   • Before: Both throwers and catchers had to know the low-level details of how to construct the same exception messages.

   • After: Both throwers and catches have to know how to call the same exception constructors.

2. The code's more mutable.

   • Before: If an exception message would be changed, a maintainer would have to track down all throwers and catchers that refer to it, then update them the same way.

   • After: If an exception message would be changed, a maintainer can just change it once.

---

Discussion

To me, it looks like the code was already generating a duplicate-exception and then checking for equality.

The logic appears to be:

1. Generate the duplicate-expression.

2. Serialize the duplicate-expression (i.e., get its exception-message).

3. Serialize the caught-expression (i.e., get its exception-message).

4. Compare the serializations to check for equality.

But instead of putting these into an object-oriented logic, these steps are all in-lined into every call site. This manual in-lining leads to the code-smell.

I suppose folks might find it strange to generate an Exception merely to check it against another (rather than throw it), but there's nothing wrong with it.

Answer 8

assertThatThrownBy(() -> shoppingCartService.payForCart(command))
        .isInstanceOf(PaymentException.class)
        .hasMessage("Cannot pay for ids [" + item.getId() + "], status is not WAITING");

Would be a more AssertJ-ish way to test the message, anyway...

You can also use hasMessageContaining if you only want to check that something is in there but don't care about the entire message

Answer 9

When comparing string literals, always use the string source.

So in your case, you need to find the Exception constructor, and either externalize the string or use a common constant version of it.