33

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?

  • 8
    I think you have answered your own question, haven't you? All your arguments are correct. – Christian Hackl Mar 4 at 13:44
  • 2
    IMO, if you must test the message (e.g., it's a very generic exception, and all the relevant information is in the message) of a thrown exception, check for a phrase, anything more than that is probably overspecified, and probably a waste of time. On the other hand, if it's a specialized exception, and the message can be standardized, then you can disallow a custom message - just pass the relevant data to the constructor, and embed it within the standardized string. And if you want to test that format, you can just do it in one place. – Filip Milovanović Mar 4 at 13:45
  • The exception is clearly wrong as there should be a full stop after the sentence. More seriously - an innocent textual fix like that shouldn't break the test. – MSalters Mar 5 at 15:19
33

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.

| improve this answer | |
  • 1
    In other words, don't make your exceptions Stringly Typed. – StackOverthrow Mar 4 at 21:30
  • They're just too dxxxxd easy for somebody [else] to change I agree with your overall stance in your answer, but wouldn't the ease of change actually be an argument in favor of writing a test if the text is, for some obscure or misguided reason, really important to the project? The test would have a comment explaining the rationale of the writer of why it would be bad to change the text. – Laurent LA RIZZA Mar 5 at 8:02
  • 1
    @LaurentLARIZZA: My point is that the text /should not/ be "really important to the project". The obvious weakness here is internationalisation. New country, new text to be tested when, in truth, what really matters is that a PaymentException was thrown for the id value(s) passed with the Exception object. For logging purposes, you should always log the whole Exception object (with its Message, Stack Trace /and/ custom properties) never /just/ the message. – Phill W. Mar 5 at 12:09
  • 2
    @PhillW. We agree. My point is: think legacy code. Imagine that, for some (pathologically but not unseenly) misguided reason, the text is really important to the application. Now someone changes it, and it breaks something seemingly unrelated in production. First thing to do : revert the change. Second thing to do : write a test to check that the exception keeps the same text. Third thing to do : add comments to explain why it's not a good idea to change it or to remove the test. Fourth thing to do : Assess how you can safely fix the system so that the exception text matters no more. – Laurent LA RIZZA Mar 5 at 13:06
  • 4
    This is a special case of the general principle that you should never put anything in an object's string representation that isn't available from the object's API. If you do, someone will try to parse it and the string representation will become a de facto API. – StackOverthrow Mar 5 at 16:29
55

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.

| improve this answer | |
  • 7
    Good answer. I would just emphasize the alternative much more. – cmaster - reinstate monica Mar 4 at 16:37
  • 36
    Well, I don't think you should have a different exception class for every single throw statement, but I think you should have a different exception class for every discernible error case. That is, insufficient balance, and invalid status are different errors, and should have different error classes (could share a common parent exception). But if you can determine insufficient balance at multiple different locations in the code, they shouldn't be different exception classes just because they're thrown at a different time. That's what stack traces in logs are for. – Cruncher Mar 4 at 16:45
  • 1
    Now that C# has finally added exception filters, I would think that instead of having many kinds of exceptions, it would be better to have one custom exception class with a function that would accept arguments related to the situations a handler would be interested in, and indicate whether the handler should catch the exception. – supercat Mar 4 at 21:15
  • 2
    @Cruncher On the one hand I totally agree, on the other defining exceptions is sooo much hassle. New file, class and you need to update catchers etc. I often give in and throw something generic because adding an exception annoys me. I feel like there should an easier way to extend that. – Christian Sauer Mar 5 at 6:55
  • 1
    @ChristianSauer I sympathize with this, I do. I do think that languages should have an easier way of targeting the catch for a thrown exception, like maybe being able to name the exception when you throw it, rather than having to predefine it. I can't count the number of times I've written raise Exception('Some Message') #TODO: Create exception class. But that doesn't mean we shouldn't be doing it. If I could write raise Exception(name='InsufficientBalanceError', message='...') it would be nice. Actually... I think I can probably build this in python. – Cruncher Mar 5 at 15:26
7

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.

| improve this answer | |
5

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"?

| improve this answer | |
1

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.

| improve this answer | |
1

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;
    }
}
| improve this answer | |
1

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.

| improve this answer | |
0
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

| improve this answer | |
-4

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.

| improve this answer | |
  • 6
    But that's useless. You'd be checking if the message thrown is the same message that would be thrown. Literally checking if the constant is a constant. At best, it verifies if the constant template is used but that's hardly useful - what if somebody mistakenly sets the constant to an empty string? it's still used but it's not terribly useful for any consumer of that message even when your test passes. – VLAZ Mar 5 at 7:45
  • The thing about constants... nobody can mistakenly set the constant to an empty string. So instead of comparing against "status is not WAITING", you'd compare against the constant TEXT_FOR_EXCEPTION_STATUS_IS_NOT_WAITING. – palin dromo Mar 5 at 21:20
  • 1
    Why would nobody be able to set it? If they are doing a refactoring and mistakenly change the sourcecode from (effectively) MY_CONSTANT = "Some text here" to (effectively) MY_CONSTANT = "" then what good is it that your test of (essentially) "" == "" passes? – VLAZ Mar 5 at 21:23
  • 2
    You've not explained what the test of "is an empty string an empty string" good for. Because that's what you're transitively testing for. Whereas I believe that a test should have use so that a pass would mean something. If it passes and you still have a problem, then you need another test...to check if the constant you're checking for has the content you expect. Which brings us back at square one. – VLAZ Mar 5 at 21:26

Not the answer you're looking for? Browse other questions tagged or ask your own question.